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: boolean option should work with strict #1996
Conversation
@jly36963 this looks like a great solution 👏 It's the end of the day and my brain is a little tired, so I'm going to wait to review fully. In the meantime, I'm going to release this, which has several of your patches in it. Thanks for the contributions. |
For sure! Glad I can help 👍 |
@jly36963 bother you to rebase? |
@@ -2206,6 +2214,15 @@ export class YargsInstance { | |||
[kSetHasOutput]() { | |||
this.#hasOutput = true; | |||
} | |||
[kTrackManuallySetKeys](keys: string | string[]) { | |||
if (typeof keys === 'string') { | |||
this.#options.key[keys] = true; |
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.
doesn't the this[kPopulateParserHintArray]('boolean', keys);
function also populate this.#options
?
I'm wondering why we need this additional kTrackManuallySetKeys
? I'm probably missing something.
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.
Example code
yargs
.command({
command: "cmd",
desc: "cmd desc",
builder: (yargs) =>
yargs
.option("opt1", {
desc: "opt1 desc",
type: "boolean",
})
.option("opt2", {
desc: "opt2 desc",
boolean: true,
})
.array("opt3")
.boolean("opt4")
.count("opt5")
.number("opt6")
.string("opt7")
.fail(() => {
console.log('failure!!!')
}),
handler: (argv) => {
console.log(argv);
},
})
.help()
.strict()
.parse("cmd --opt1 --opt2 --opt3 foo bar baz --opt4 --opt5 --opt6 3 --opt7 oof");
Reason for the change
yargs.option
has this line:
this.#options.key[keys] = true;
This line populates options.key
.
Before my fix, calling yargs.boolean
(or array, count, string, number, etc) was missing this behavior.
To highlight the difference in behavior between .option()
and .boolean()
, I added opt1 and opt2 using .option()
. In the "Without fix" section below, you should see that options.key
has opt1 and opt2, but is missing the others.
Explanation
I logged options (yargs.getOptions()
) inside of validation.isValidAndSomeAliasIsNotNew
Without the fix, options.key
is missing keys, so isValidAndSomeAliasIsNotNew
fails.
Without fix:
{
local: [],
configObjects: [],
array: [ 'opt3' ],
boolean: [ 'version', 'help', 'opt1', 'opt2', 'opt4' ],
string: [ 'opt7' ],
skipValidation: [],
count: [ 'opt5' ],
normalize: [],
number: [ 'opt6' ],
hiddenOptions: [],
narg: {},
key: { version: true, help: true, opt1: true, opt2: true },
alias: {},
default: {},
defaultDescription: {},
config: {},
choices: {},
demandedOptions: {},
demandedCommands: {},
deprecatedOptions: {},
envPrefix: undefined,
__: [Function: bound __],
configuration: {}
}
WIth fix
{
local: [],
configObjects: [],
array: [ 'opt3' ],
boolean: [ 'version', 'help', 'opt1', 'opt2', 'opt4' ],
string: [ 'opt7' ],
skipValidation: [],
count: [ 'opt5' ],
normalize: [],
number: [ 'opt6' ],
hiddenOptions: [],
narg: {},
key: {
version: true,
help: true,
opt1: true,
opt2: true,
opt3: true,
opt4: true,
opt5: true,
opt6: true,
opt7: true
},
alias: {},
default: {},
defaultDescription: {},
config: {},
choices: {},
demandedOptions: {},
demandedCommands: {},
deprecatedOptions: {},
envPrefix: undefined,
__: [Function: bound __],
configuration: {}
}
Addresses: #1694
When defining an option with
yargs.option
, this line is called:This affects the behavior in
validation > unknownArguments > isValidAndSomeAliasIsNotNew
. Without the tracking of set keys,unknownArguments
will fail (as described in the issue).I turned that line into a reusable function so that
yargs.option
and the array/boolean/count/number/string methods have consistent behavior when usingyargs.strict