-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
React to wrong library usage #1917
Closed
Closed
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
97fda7c
Throw errors & show warnings on wrong library use
aweebit ac955dc
Add note on inherited settings to docs
aweebit 0553d32
Remove warning detail only relevant when thenables from argParsers ar…
aweebit 55b9146
Improve shared subcommand error
aweebit dfe2fc7
Improve docs about inherited settings
aweebit 3572657
Place async hook / action warning better
aweebit adde7ff
Remove unused _asyncParsing property
aweebit 8ca1459
Revert docs change moved to #1925 to unclutter PR file changes
aweebit c958d2b
Remove NODE_ENV check
aweebit File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I care enough about this error to add. For interest, did you hit this in real world usage?
If I did add it, I think I would rather have some small code repetition than an extra level of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems a little weird to add the error in
addCommand()
but not this one. But everything could be fixed by implementing the suggestion from the first comment. Why do you dislike it? I think it is very easy to implement. If I'm not mistaken, it would require even fewer lines of code than the combination of this warning and the error inaddCommand()
(cannot check it right now unfortunately since I'm on a small trip and haven't taken my laptop with me). Only the error frompassThroughOptions()
would need to be turned into a warning in_parseSubroutibe()
, and something done about theparent
use inoutputHelp()
(maybe should just leave theparent
assignment incommand()
/addCommand()
).No, just trying on the library developer mindset and thinking of different ways it could be used 🙂
More shared code can be added to
_parseSubroutibe()
later, like after the suggestion from the first comment is implemented or #1919 is merged.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The high level author error that does sometimes happen is chucking
parse()
on the end of the subcommand definition. This would catch that particular case. It does not come up often and I wonder if there are valid use cases for calling.parse()
down tree in fancy custom programs.To encourage a usage pattern that avoids several potential problems, I use this style as much as possible in the hopes people will just copy it:
program
.parse()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we propagate
parse()
/parseAsync()
calls to the root command? Then chucking a call at the end of a subcommand definition would not be a problem, and we would not need the warning since subcommands would not be usable as stand-alones.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggestion from the first comment would also become irrelevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... No, I think not. Either the user is intentionally doing something interesting outside normal usage, or they are confused about how Commander works. Doing something different that might be what they intended is masking the situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of now, there are no meaningful semantics / intended usage patterns for
parse()
/parseAsync()
calls on subcommands anyway, and any such call should be considered wrong. The only meaningful semantics for such a call I can think of are exactly that, propagating it to the root command. So why not assign a meaning to calls that currently have no meaning at all, giving the developer the freedom to chuck a parse call at the end of a subcommand chain? That is not doing something different that might be what they intended, that is doing the only meaningful thing when receiving such a call instead of doing complete nonsense (what the library currently does) or having to come up with ways to tell the user they are using the library in a wrong way (what this PR is currently all about).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two days later, I am having second thoughts about my comment on the suggestion from the first comment becoming irrelevant. I think I was wrong saying that, it will still be relevant because it deals with parse calls on a parent command that has lost ownership of a subcommand, rather than on a subcommand. So I would still like to hear why you disliked that suggestion. Implementing it would make the warning this thread is about unnecessary since parsing subcommands as stand-alones would become possible. Could still leave the warning for the case when a parse call is chucked a the end of a subcommand chain, though, for example by adding a
_parseCallsUndesired
field with the default value offalse
to theCommand
class and setting it totrue
for subcommands created incomnand()
.