[AmirSa12]

related to the discussions in #154

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@bomb.sh/tab@155

commit: 4564f99

[changeset-bot]

⚠️ No Changeset found

Latest commit: 4564f99

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[AmirSa12]

A one-time behavioral probe could also work i think.
@shadowspawn any thoughts?

[shadowspawn]

A one-time behavioral probe could also work i think.

Do you mean to avoid accessing the private _enablePositionalOptions property? I think what you have done with an explicit pattern for accessing the private property (CommandWithPositionalOptions) is good enough for now.

I'll think about how to expose this state, but it will of course only be usable in new versions. There isn't a strong reason for having this state/property private. (I tend to make new properties private until there is a demand for them, to simplify internal changes and keep the API smaller.)

[shadowspawn]

Thanks for working on this! Would you like a review?

[AmirSa12]

Thanks for working on this! Would you like a review?

Yes, please!🙌🏼
Thanks you!

[shadowspawn]

Working my way slowly through the new code. Is the goal for now to leave the old code largely untouched and behaviour unchanged unless setOptionLookupMode is called?

1. OptionLookupMode could also have local-first (for other parsers) for more symmetrical code but that would make it harder to tell from diff inspection that old behaviour unchanged.

2. There is a mixture of a new shim calling old or new code based on OptionLookupMode, old routines calling new routines instead if OptionLookupMode set, and in-place code changes in old routines if OptionLookupMode set. Which isn't a problem as such, just checking the intentions. :-)

[shadowspawn]

The code is using this WeakMap to lookup a value being held separate from the RootCommand. Is there a reason for that, rather than just having a property on the RootCommand? (It isn't wrong but I am wondering if this pattern means I didn't understand something, or might be a left-over from an early approach when it was needed.)

(In Commander technically the lookup mode can be set on any command in hierarchy and not just the RootCommand, but in practice usually set the same on all commands. I am fine with only being set on RootCommand.)