[AmirSa12]

related to the discussions in #154

[pkg-pr-new]

Open in StackBlitz

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

commit: 7056504

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7056504

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. I think OptionLookupMode could also have local-first (for other parsers) for more symmetrical code, but that would make it harder to tell from diff inspection in this PR that old behaviour unchanged, so could do that in separate PR.

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.)

[AmirSa12]

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.)

Good point... I initially used the WeakMap to keep adapter-specific lookup state separate from RootCommand and avoid exposing another property. But I guess there is no real benefit here. I moved it onto RootCommand

[shadowspawn]

I have carefully read through the new code and makes sense. I like optionCommandsFor as a way of localising the lookup mode and letting higher level code just loop over the candidate commands, nice.

I'll work through the tests and do some runtime manual tests too to confirm coverage. I was just taking a light look at the fixture and can see some thoughtful layers of options for probes!

[shadowspawn]

(Full disclosure. I am wondering about having a go at making Commander parse options with local-first priority by default in a future major version. Prompted in part by the discussions and investigations here! Not a high priority though and may turn out to be too hard.)

[shadowspawn]

Unit tests and manual checks of behaviour look good.

[AmirSa12]

I have been AFK and just went through your review and suggestions, thanks a lot! that is really helpful. ❤️

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. I think OptionLookupMode could also have local-first (for other parsers) for more symmetrical code, but that would make it harder to tell from diff inspection in this PR that old behaviour unchanged, so could do that in separate PR.
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. :-)

Yes, the intention is to preserve the existing behavior unless an adapter configures optionLookupMode.
The commander adapter selects parent-first by default and local when positional options are enabled.
LMK if you have other ideas!

I applied your suggestions, lmk if something is missing.