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

aweebit · 2023-07-28T21:03:58Z

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…

Add async custom processing support. Add chainArgParserCalls() for configuration. Additionally await thenable implicit and default option values and thenable default argument values #1915 (_asyncParsing is used in this PR instead of simply adding a boolean variable indicating whether the command is currently being parsed because it is assumed the PRs are merged together. If they are not, use a boolean variable instead)

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

React to wrong library usage #1917

Public setOptionValueWithSource() method only accepts 'config' as source

Remove overlap between setOptionValue() and setOptionValueWithSource(). Make the latter's source parameter required.

shadowspawn · 2023-07-29T00:37:51Z

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

shadowspawn · 2023-07-29T00:55:51Z

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

shadowspawn · 2023-07-29T05:28:00Z

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

Co-authored-by: John Gee <john@ruru.gen.nz>

aweebit · 2023-07-29T06:44:06Z

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 · 2023-07-29T06:48:44Z

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 · 2023-07-29T07:05:03Z

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

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

aweebit · 2023-07-29T07:07:59Z

implicitly set option values (might be ok)

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

shadowspawn · 2023-07-29T07:09:08Z

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 · 2023-07-29T11:52:51Z

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

Implemented in 94e439d.

...by adding missing parameter in the _parseCommand() call in _dispatchSubcommand().

aweebit · 2023-07-29T23:57:18Z

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

shadowspawn · 2023-07-30T02:12:07Z

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

shadowspawn · 2023-07-30T03:17:06Z

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

aweebit · 2023-07-30T03:37:42Z

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

Fixed in 4a7bc41.

aweebit · 2023-07-30T04:00:26Z

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 · 2023-07-30T09:10:13Z

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.

Additionally fix comment impreciseness

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

shadowspawn · 2023-08-01T05:17:46Z

lib/command.js

-    unknown = parsed.unknown;
-    this.args = operands.concat(unknown);
+  _parseCommand(operands, unknown, async) {
+    this._asyncParsing = async;


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

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.

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

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

aweebit · 2023-08-01T21:45:43Z

TODO

Take closing comments from Prevent opts() from unsafely exposing a private object and expose a proxy instead #1921 into consideration and rethink returning a proxy from the constructor

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 · 2023-08-03T09:36:11Z

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.

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

Borrowed from tj#1917 and tj#1919.

shadowspawn · 2023-08-08T02:25:52Z

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 · 2023-08-12T16:18:57Z

⚠⚠⚠ 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

Add missing test and support help options with only the short flag in Help.visibleOptions() #1935 (might want to decouple from #1929)
Never call preSubcommand hooks on help command #1956

Enhancements / changes in behavior

ChangeLog type: Added / Changed

Changes in behavior fixing open issues

Prevent aliases already in use and subcommands with such an alias or the name already in use from being added #1924 (fixes #1903)

Misc

Prevent options with flags already in use from being added #1923 (closely related to #1924 from right above)
Refactor help option and improve option parsing from parent when default command is invoked implicitly #1934
Use createOption() in helpOption() to support custom option flag extraction (+ various improvements) #1929 (a subset of #1934 from right above)
Make copyInheritedSettings() recursive #1922
Add .strictOptionalOptionArguments() for POSIX-style handling of options with optional option-arguments #1951 (fixes #1901, quite robust and thus ready for review in my opinion, only tests missing)

Using proxies

Clean up state from previous parse call when calling parse() / parseAsync() #1919 (awaiting feedback – might dump proxy use altogether) (partially fixes #1916 and a lot of old issues)

With warnings about presumably wrong library usage

Warn about async calls in .parse() #1940 (currently closed, but additional motivation has been provided in #1940 (comment)) (partially fixes #1916)
Add support for sharing and stand-alone parsing of subcommands. Warn about parse calls on commands added with .command() #1938 (includes an arguably questionable enhancement to parent handling, so might require more time to think about than the previous PR, but the warning is very, very, VERY useful) (partially fixes #1916)
Warn about obscured help option #1931 (not as useful as the previous one because it is likely the develop is okay with the help option being obscured, but a good complement to #1923)
Add .suppressWarnings() for warnings in #1915 #1931 #1938 #1940 #1955 (a reasonable addition since all warnings introduced in the PRs above are for usage patterns that are not always wrong, just often connected with a developer mistake)

Pure refactors and other changes of low importance

Only for contributors

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

shadowspawn · 2023-08-18T08:19:27Z

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.

aweebit added 4 commits July 28, 2023 22:49

Clean up state from previous parse call

3c6ef71

Improve persistent option value logic

b6d53b2

Improve logic for option values supplied in user code

437291f

Public setOptionValueWithSource() method only accepts 'config' as source

Remove public method functionality overlap

fbe911f

Remove overlap between setOptionValue() and setOptionValueWithSource(). Make the latter's source parameter required.

aweebit mentioned this pull request Jul 28, 2023

On command and subcommand reusability and setting inheritance #1916

Closed

shadowspawn reviewed Jul 29, 2023

View reviewed changes

lib/command.js Outdated Show resolved Hide resolved

shadowspawn reviewed Jul 29, 2023

View reviewed changes

lib/command.js Outdated Show resolved Hide resolved

aweebit mentioned this pull request Jul 29, 2023

React to wrong library usage #1917

Closed

aweebit and others added 2 commits July 29, 2023 09:22

Do not clear persistent state

ebf672a

Co-authored-by: John Gee <john@ruru.gen.nz>

Clarify that default option values are persistent

7fd0b22

aweebit added 3 commits July 29, 2023 14:02

Fix user-supplied option value source type

940b111

Use string instead of OptionValueSource

52d5885

OptionValueSource is only meaningful as defined in commander-js/extra-typings#3

Make user-supplied option values non-persistent while parsing

94e439d

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.

Add non-persistent user-supplied option support for subcommands

274be1a

...by adding missing parameter in the _parseCommand() call in _dispatchSubcommand().

shadowspawn reviewed Jul 30, 2023

View reviewed changes

lib/command.js Outdated Show resolved Hide resolved

aweebit added 2 commits July 30, 2023 06:18

Fix conditional method call in setOptionValueWithSource()

56eed63

Reset subcommand parse state when dispatching

4a7bc41

Change OptionValueSource to string in type tests

fcc97e2

aweebit added 2 commits July 30, 2023 12:00

Make comment about options with conflicting names more precise

829655d

aweebit added 2 commits July 30, 2023 15:29

Use right variable name

633e4e4

Fully support options with names conflicting with instance properties

be00f59

Additionally fix comment impreciseness

aweebit mentioned this pull request Jul 30, 2023

Prevent opts() from unsafely exposing a private object and expose a proxy instead #1921

Closed

shadowspawn reviewed Aug 1, 2023

View reviewed changes

aweebit mentioned this pull request Aug 1, 2023

Add async custom processing support. Add chainArgParserCalls() for configuration. Additionally await thenable implicit and default option values and thenable default argument values #1915

Closed

aweebit marked this pull request as draft August 2, 2023 07:19

aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 5, 2023

Add subroutine for common parse call code

d1686db

Borrowed from tj#1917 and tj#1919.

shadowspawn added the on hold Not a current focus due to dependencies on other PR or number of other PR label Aug 8, 2023

This was referenced Aug 13, 2023

Fix option value source type #1963

Closed

Pull request catalog by @aweebit #1964

Closed

shadowspawn closed this Aug 18, 2023

shadowspawn removed the on hold Not a current focus due to dependencies on other PR or number of other PR label Aug 18, 2023

aweebit mentioned this pull request Aug 18, 2023

[Feature Request] postParse hooks #1976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aweebit commented Jul 28, 2023 •

edited

shadowspawn commented Jul 29, 2023

shadowspawn commented Jul 29, 2023

shadowspawn commented Jul 29, 2023

aweebit commented Jul 29, 2023 •

edited

aweebit commented Jul 29, 2023 •

edited

aweebit commented Jul 29, 2023

aweebit commented Jul 29, 2023

shadowspawn commented Jul 29, 2023

aweebit commented Jul 29, 2023 •

edited

aweebit commented Jul 29, 2023

shadowspawn commented Jul 30, 2023

shadowspawn commented Jul 30, 2023

aweebit commented Jul 30, 2023

aweebit commented Jul 30, 2023

aweebit commented Jul 30, 2023 •

edited

shadowspawn Aug 1, 2023 •

edited

aweebit Aug 1, 2023

shadowspawn Aug 1, 2023

aweebit Aug 1, 2023 •

edited

aweebit commented Aug 1, 2023

aweebit commented Aug 3, 2023 •

edited

shadowspawn commented Aug 8, 2023

aweebit commented Aug 12, 2023 •

edited

shadowspawn commented Aug 18, 2023

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

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

Conversation

aweebit commented Jul 28, 2023 • edited

ChangeLog

Changed

Fixed

Peer PRs

Merge this PR after…

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

shadowspawn commented Jul 29, 2023

shadowspawn commented Jul 29, 2023

shadowspawn commented Jul 29, 2023

aweebit commented Jul 29, 2023 • edited

aweebit commented Jul 29, 2023 • edited

aweebit commented Jul 29, 2023

aweebit commented Jul 29, 2023

shadowspawn commented Jul 29, 2023

aweebit commented Jul 29, 2023 • edited

aweebit commented Jul 29, 2023

shadowspawn commented Jul 30, 2023

shadowspawn commented Jul 30, 2023

aweebit commented Jul 30, 2023

aweebit commented Jul 30, 2023

aweebit commented Jul 30, 2023 • edited

shadowspawn Aug 1, 2023 • edited

Choose a reason for hiding this comment

aweebit Aug 1, 2023

Choose a reason for hiding this comment

shadowspawn Aug 1, 2023

Choose a reason for hiding this comment

aweebit Aug 1, 2023 • edited

Choose a reason for hiding this comment

aweebit commented Aug 1, 2023

TODO

aweebit commented Aug 3, 2023 • edited

shadowspawn commented Aug 8, 2023

aweebit commented Aug 12, 2023 • edited

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

Patches

Enhancements / changes in behavior

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 commented Aug 18, 2023

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

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

aweebit commented Jul 28, 2023 •

edited

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

aweebit commented Jul 29, 2023 •

edited

aweebit commented Jul 29, 2023 •

edited

aweebit commented Jul 29, 2023 •

edited

aweebit commented Jul 30, 2023 •

edited

shadowspawn Aug 1, 2023 •

edited

aweebit Aug 1, 2023 •

edited

aweebit commented Aug 3, 2023 •

edited

aweebit commented Aug 12, 2023 •

edited