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
fix: strict should fail unknown arguments #1977
Conversation
@jly36963 thanks for digging into this, will make an effort to review this week 👍 It's great to start squashing some of the long standing bugs. |
@biggianteye, @brlodi, I've published @jly36963's fix to
|
I've just tested this version with the code I used in the bug report and it is now working as expected. Thanks!
|
The problems which previously motivated me to switch from commander to yargs have been solved (by the addition of .exitOverride() and .configureOutput()). Commander has more flexibility than yargs for option processing using .on('option'), which can be used for options which are sensitive to ordering or have more complicated handling. It is also much simpler and has less exotic behavior that needs to be disabled (see all the yargs options which were set). Finally, it is about 1/6 the total size. Also, for this project specifically, yargs@17.1.0 broke positional argument parsing due to yargs/yargs#1977. Since the argument is optional, `.demand()` is not appropriate (it's also deprecated). It appears the correct fix would be to add a default command and define the positional argument on that. However, I'm done dealing with yargs breakage, and switching to Commander will a better return on effort. Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
Addresses: #1861
As always, let me know if I need to change/add/fix anything.
Issue
This should fail:
Neither
unknownArguments
norunknownCommands
catch extra args whencommandKeys.length
is 0.Fix
I added a block to check if there are any elements in
argv
that aren't incurrentContext.commands
. I made sure to tolerate args whenyargs.demand(number)
is called, when the number of args falls within the min/max range.This shouldn't fail:
Note
I don't understand the
demand
logic well (as it's deprecated and not in the docs), so I'm not sure if the slice in this line will always be correct:I saw that a few other places do something similar.