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

Add support for sharing and stand-alone parsing of subcommands. Warn about parse calls on commands added with .command() #1938

Conversation

aweebit
Copy link
Contributor

@aweebit aweebit commented Aug 5, 2023

An extension of #1937 showcasing a better version of the changes I suggested in #1917 (comment).

This was originally a proof of concept PR that expected to give new life to #1917 and #1924.

Now, I would like this PR and #1940 to supersede #1917. See #1917 (comment) for details.

Partially fixes #1916.

ChangeLog

Added

  • support for sharing and stand-alone parsing of subcommands
  • warnings about parse calls on commands added with .command()

Motivation

  • gives meaning to currently meaningless parse calls on subcommands
  • the meaning given is what most users probably expect
  • makes the error introduced in #1917 unnecessary, and the warning introduced there not absolutely necessary but just useful for chains with .command()

Implementation details

  • backwards compatibility is preserved: the behavior has not changed in code using the current and previous versions of the library as intended (only adding commands to one parent and parsing from the parent)
  • the implementation is quite low-effort
    • very few code changes
    • unlikely something could go wrong
    • here is a summary:
      • additionally set parent to the invoking parent command at the beginning of _parseCommand()
      • add parents to store all parent commands so that no parent checks have to be moved to parse calls, which I wrongly assumed would be necessary in my comment to #1924 (see how _checkForBrokenPassThrough() is implemented in this PR, for example)
      • add _implicitlyCreated indicating whether the command was created with .command()
      • check its value to warn the developer about parse calls at the end of a .command() chain

Peer PRs

Warnings need to be consistent with…

Requires changes when merged with…

@shadowspawn
Copy link
Collaborator

Nice clear labelling as "Proof of concept" and "Draft", and good description to explain.

The approach of having a stable forward/subcommand link while wiring up the reverse/parent links while parsing is an interesting idea. The wiring itself is indeed "low effort".

The downside is a more complex model for maintainers and to a lesser extent authors, to support some exotic reuse configurations.

I am comfortable with the current simple model of one stable tree with matching forwards and backwards links. This POC is not something I am interested in proceeding with at this time.

@aweebit aweebit changed the base branch from develop to release/12.x August 5, 2023 06:43
@aweebit
Copy link
Contributor Author

aweebit commented Aug 5, 2023

Hmm, I expected the already merged commits to not be shown anymore after changing base and merging with the new one. Well, at least the file changes are not cluttered anymore.

.currentParent is expected to never be different from .parent in
existing code using the library as intended.
@aweebit
Copy link
Contributor Author

aweebit commented Aug 5, 2023

The downside is a more complex model for maintainers and to a lesser extent authors, to support some exotic reuse configurations.

What kind of authors would the more complex model affect? Authors of libraries extending the functionality of Commander somehow? I don't think any maintenance complications are implied for authors of CLI applications using Commander as currently intended (only adding commands to one parent and parsing from that parent) since the change is fully backwards compatible and invisible in such cases (I have even got rid of currentParent in the most recent commit because it was redundant). And if they do parse subcommands from different parents or as stand-alones, then they know what they are doing.

I am comfortable with the current simple model of one stable tree with matching forwards and backwards links. This POC is not something I am interested in proceeding with at this time.

One reason I want to support the exotic reuse configurations is because right now, parsing subcommands as stand-alones is allowed despite being a meaningless operation (since the .parent is not reset to null), so it would be reasonable to forbid it, but there is no good way to do it because parse calls are only supposed to throw errors originating in user code (argParsers, hooks and actions), so warnings have to be used instead, and I would like to avoid that as much as possible.

Replaces getCommandAndParents():
- turned into an instance method
- used "ancestors" instead of "parents" because it is more specific and
  to avoid confusion with the recently introduced parents array
@aweebit aweebit changed the title [Proof of concept] Add support for sharing and stand-alone parsing of subcommands. Warn about parse calls on commands added with .command() [Proof of concept] Add support for sharing and stand-alone parsing of subcommands. Warn about parse calls on commands added with .command() Aug 5, 2023
Replaces getCommandAndParents():
- turned into an instance method
- used "ancestors" instead of "parents" because it is more precise

(cherry picked from commit aa280af)
@aweebit
Copy link
Contributor Author

aweebit commented Aug 5, 2023

The downside is a more complex model for maintainers

I can agree some overhead is introduced for contributors, but really insignificant. Would just have to think whether iterating over parents is more appropriate when using parent in code, that's it.

A corresponding type test for parents has been added.
@aweebit aweebit marked this pull request as ready for review August 5, 2023 10:31
@aweebit aweebit changed the title [Proof of concept] Add support for sharing and stand-alone parsing of subcommands. Warn about parse calls on commands added with .command() Add support for sharing and stand-alone parsing of subcommands. Warn about parse calls on commands added with .command() Aug 5, 2023
@aweebit
Copy link
Contributor Author

aweebit commented Aug 5, 2023

I would now like this PR and #1940 to supersede #1917. See #1917 (comment) for details.

@aweebit
Copy link
Contributor Author

aweebit commented Aug 7, 2023

Just a quick note. I keep finding myself putting .parse() at the end of chains that have .command() in them by mistake when writing tests and demos for my pull requests. Introducing warnings about such parse calls would indeed be very, very useful.

@shadowspawn
Copy link
Collaborator

This is a useful note, thanks: #1938 (comment)

I added one previous warning because of a mistake I kept making: #1655

@shadowspawn shadowspawn added the on hold Not a current focus due to dependencies on other PR or number of other PR label Aug 8, 2023
Only call when all elements are to be iterated.
Resort to manual iteration via .parent in other cases.
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 11, 2023
aweebit added a commit to aweebit/commander.js that referenced this pull request Aug 11, 2023
Made it less strict: the library user might know better.
@shadowspawn
Copy link
Collaborator

These changes are not something I am interested in proceeding with.

@shadowspawn shadowspawn removed the on hold Not a current focus due to dependencies on other PR or number of other PR label Aug 20, 2023
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.

None yet

2 participants