Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up state from previous parse call when calling parse() / parseAsync() #1919

Closed
wants to merge 18 commits into from

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Jul 28, 2023

The better solution for #438 #1565 #1819 #1829 #1916 suggested in #1916.

Outdated

While implementing it, I noticed that the type restriction for the source parameter of the public setOptionValueWithSource() method are too loose. The only really supported source value seems to be 'config' as of now, so I changed the type to this single value. This has the benefit of preventing messing up the internal logic by supplying option values with the source being one of 'default', 'cli', 'env' and 'implied' (at least when using TypeScript, might also want to add a real check in code).

ChangeLog

Changed

  • Breaking: previous parse() / parseAsync() call state is cleaned up in the beginning of a new call (enables command reuse for testing, REPL etc.)

Fixed

  • option value source type (set to string | undefined to enable custom source use)

Peer PRs

Merge this PR after…

Parse call subroutine (_parseSubroutine()) needs to be consistent with…

Public setOptionValueWithSource() method only accepts 'config' as source
Remove overlap between setOptionValue() and setOptionValueWithSource().
Make the latter's source parameter required.
@shadowspawn
Copy link
Collaborator

While implementing it, I noticed that the type restriction for the source parameter of the public setOptionValueWithSource() method are too loose. The only really supported source value seems to be 'config' as of now, so I changed the type to this single value.

It is intended that the author can use any value, whether one of the ones Commander uses internally, or a custom value which means something to the author but not to Commander. The option-source was added in part to make configuration files easier to implement by user, and in part to support adding environment variables.

It is a bit difficult for Commander to decided how to treat unknown values, and in general I dislike adding support for speculative uses. In this case I was encouraged by having multiple use cases I wanted to support in mind from the start, and it then proved useful for implementing .conflicts() too.

lib/command.js Outdated Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator

To be clear, I don't think I'll want this! So choose how much further work to do accordingly.

But I'll work through and make comments, and interested in whether it could work. I find there is only so much I can work out in theory, and usually learn more once I try writing or running the code.

lib/command.js Outdated Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator

On to the reset itself.

There are two specific things which are not working currently in this PR:

  • resetting default values
  • resetting if legacy .storeOptionsAsProperties() is enabled. This is definitely legacy but has not been deprecated yet. It could be forbidden rather than supported. Other than setting option values to undefined, there isn't a high-level way of "deleting" a value so hard to fully restore property state.

High level things I am dubious about include

  • author set option values: before parse, from hooks, after parse
  • implicitly set option values (might be ok)

If aiming for a high level model that a second parse() starts from the same state as the first parse, a more direct model might be saving the state on first entry and resetting state on subsequent entries. This bypasses trying to decide whether to persist changes as they occur.

An entirely different design is that calling .parse() does not change any state and returns the parse result. This is very much not the way Commander is written.

I looked back at some of the issue and PR history. There was an early PR for a reset in #499. It is way simple, but does have one interesting insight that the "known" options are available from the options array.

To be clear, I don't think I'll want this! So choose how much further work to do accordingly.

Warning still applies. 😄

@aweebit
Copy link
Contributor Author

aweebit commented Jul 29, 2023

  • resetting default values

Default values were actually reset. I made it more clear in 7fd0b22.

  • resetting if legacy .storeOptionsAsProperties() is enabled. This is definitely legacy but has not been deprecated yet. It could be forbidden rather than supported. Other than setting option values to undefined, there isn't a high-level way of "deleting" a value so hard to fully restore property state.

How about storing all properties of this as persistent option values in this mode?

Update: Non-persistent options set by directly adding properties to the Command instance (e.g. from within hooks, see my later comment below) could be registered by using an ES6 Proxy and later deleted when resetting state.

Update: Using proxies actually seems like a brilliant idea to me. All code but that of the proxy handler could be simplified to behave as if only modern options were supported, and then the handler methods would simply choose whether to access the Command instance or its _optionValues based on whether the instance has the looked-up property. And a big advantage is that overwriting instance properties could be prevented with almost zero effort.

I think cases like this are exactly what proxies are supposed to be used for.

@aweebit
Copy link
Contributor Author

aweebit commented Jul 29, 2023

or a custom value which means something to the author but not to Commander.

I did not think those were supported because of the OptionValueSource use in typings.

@aweebit
Copy link
Contributor Author

aweebit commented Jul 29, 2023

  • author set option values: before parse, from hooks, after parse

Could make setOptionValueWithSource() call _setNonPersistentOptionValueWithSource() when run from hooks.

@aweebit
Copy link
Contributor Author

aweebit commented Jul 29, 2023

  • implicitly set option values (might be ok)

They depend on input so should definitely be left non-persistent.

@shadowspawn
Copy link
Collaborator

or a custom value which means something to the author but not to Commander.

I did not think those were supported because of the OptionValueSource use in typings.

Sorry about that. I was looking into that before I saw your message as I was expecting the types would make it clear and surprised when I found explicit list. Took me a while to find the clarification which I have trialed first in companion repo: commander-js/extra-typings#3

OptionValueSource is only meaningful as defined in
commander-js/extra-typings#3
Relevant for setOptionValue() and setOptionValueWithSource() calls in
hooks and actions.

_asyncParsing is stored to avoid redundant property when merged with
tj#1915 or tj#1917.
@aweebit
Copy link
Contributor Author

aweebit commented Jul 29, 2023

Could make setOptionValueWithSource() call _setNonPersistentOptionValueWithSource() when run from hooks.

Implemented in 94e439d.

...by adding missing parameter in the _parseCommand() call in
_dispatchSubcommand().
@aweebit
Copy link
Contributor Author

aweebit commented Jul 29, 2023

Sorry have been editing code from mobile, gonna check what's wrong when I come home.

@shadowspawn
Copy link
Collaborator

No rush. (And somewhat related, I have had an unusual amount of time available in the last week, I will be less responsive this week.)

lib/command.js Outdated Show resolved Hide resolved
@shadowspawn
Copy link
Collaborator

Reset is not being called in subcommands, but still ok as a proof of concept with root command.

@aweebit
Copy link
Contributor Author

aweebit commented Jul 30, 2023

Reset is not being called in subcommands, but still ok as a proof of concept with root command.

Fixed in 4a7bc41.

@aweebit
Copy link
Contributor Author

aweebit commented Jul 30, 2023

All checks should pass now.

I think the only problem left is the one with options-as-properties. Do you think I should give the idea with proxies a try? Or is there maybe a reason why you would not want to use proxies in the library?

…constructor

Required for options-as-properties support in resetParseState().
Has the added benefit of supporting (in a limited way) option names
conflicting with instance's properties even when options-as-properties
are enabled.
@aweebit
Copy link
Contributor Author

aweebit commented Jul 30, 2023

The command constructor now returns a proxy as suggested.

I personally think the proxy use is meaningful even outside the context of this PR. It handles the options with names that conflict with instance properties better and prevents all problems with options-as-properties such as the one in this PR in the future.

aweebit added a commit to aweebit/commander.js that referenced this pull request Jul 30, 2023
…constructor

Borrowed from tj#1919.
Makes _optionValues the only true storage for option values.
Has the added benefit of supporting option names conflicting with
instance's properties even when options-as-properties are enabled.
unknown = parsed.unknown;
this.args = operands.concat(unknown);
_parseCommand(operands, unknown, async) {
this._asyncParsing = async;
Copy link
Collaborator

@shadowspawn shadowspawn Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the _asyncParsing property bring used to track whether in the middle of a parse, rather than "async"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for motivation see commit message of 94e439d. #1917 does not use _asyncParsing anymore, but there would still be a nasty redundancy that I think would not even result in a merge conflict when #1915 is merged, which I do hope will happen eventually. Maybe it is not such a good idea to keep other PRs in mind, though. I am not sure since I have almost zero experience with contributing to open source.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is not such a good idea to keep other PRs in mind, though.

In my opinion it is ok to keep them in mind, and helpful to warn about potential merge issues.

(Even ok in my opinion to suggest it should land after another PR, but then be prepared for a rewrite if that other one gets rejected!)

Copy link
Contributor Author

@aweebit aweebit Aug 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added #1915 as a peer PR, analogous to what I did for #1923 and #1924.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 1, 2023

TODO

@aweebit aweebit marked this pull request as draft August 2, 2023 07:19
Borrowed from eb142d8 that was supposed to land in the now-closed tj#1921.

The change ensures the this object used in Command is the proxy from the
very beginning, even in the constructor. This solves pretty much all
issues with returning the proxy from a constructor. For example, private
fields can be used now. More details available at:
tj#1921 (comment)

Additionally, the ownKeys() trap and wrong spelling in comments have
been fixed (the former change being borrowed from fc927c8).
@aweebit
Copy link
Contributor Author

aweebit commented Aug 3, 2023

  • resetting if legacy .storeOptionsAsProperties() is enabled. This is definitely legacy but has not been deprecated yet. It could be forbidden rather than supported. Other than setting option values to undefined, there isn't a high-level way of "deleting" a value so hard to fully restore property state.

Your comment from #1921 reads:

If you say this discrepancy is desired because keeping the old approach as backwards compatible as possible is

Effectively yes. The old approach is still available for backwards compatibility, not as an alternative mechanism for accessing options.

The way I interpret this is that supporting modern features in the legacy mode is of low importance. So how about we just resort to warnings in repeated parse calls (similar to those in #1917) when options-as-properties are enabled so that proxy use can be avoided? (I still think the solution with proxies is better, though.)

Update: I am also not sure you won't miss my new comment in #1921 in the avalanche of new PRs, especially considering it is now closed, so if by any chance you haven't seen the notification for it, please check it out.

aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 3, 2023
The _version and _versionOptionName properties are initialized to
undefined in the constructor. The reasoning is
- to make them visible when the Command instance is logged into the
console before .version() is called, and not only appear after suddenly
- to not break anything if the proxy use for consistent option value
handling in the legacy setOptionValueWithSource mode suggested in tj#1919
(and previously in tj#1921) is adopted
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 5, 2023
@shadowspawn
Copy link
Collaborator

avalanche of new PRs

Yes. Yes indeed. 😢

Which PR would you like me to look at first? I am going to mark the others as on hold.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 12, 2023

⚠⚠⚠ The list has been moved to #1964 and won't be updated here anymore! ⚠⚠⚠


Which PR would you like me to look at first? I am going to mark the others as on hold.

It does not really matter that much to me, but if I were to write a PR list sorted by the importance of the changes, it would look something like this:

Patches

ChangeLog type: Fixed

Enhancements / changes in behavior

ChangeLog type: Added / Changed

Changes in behavior fixing open issues

Misc

Using proxies

With warnings about presumably wrong library usage

Pure refactors and other changes of low importance

Only for contributors

Should be superseded

Closed but might need reconsideration


Issues that might need a PR


Completed


⚠⚠⚠ The list has been moved to #1964 and won't be updated here anymore! ⚠⚠⚠

@shadowspawn
Copy link
Collaborator

Invested quite a bit in this PR before the avalanche of new PRs.

In the first instance, I would like to improve the documentation for the existing very simple pattern. Make a new instance for each parse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants