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

fix: boolean option should work with strict #1996

Merged
merged 9 commits into from Sep 23, 2021
Merged

fix: boolean option should work with strict #1996

merged 9 commits into from Sep 23, 2021

Conversation

jly36963
Copy link
Contributor

@jly36963 jly36963 commented Aug 1, 2021

Addresses: #1694

When defining an option with yargs.option, this line is called:

this.#options.key[key] = true;

This affects the behavior invalidation > 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 using yargs.strict

@bcoe
Copy link
Member

bcoe commented Aug 4, 2021

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

@jly36963
Copy link
Contributor Author

jly36963 commented Aug 5, 2021

Thanks for the contributions.

For sure! Glad I can help 👍

@bcoe
Copy link
Member

bcoe commented Sep 5, 2021

@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;
Copy link
Member

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.

Copy link
Contributor Author

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: {}
}

@jly36963 jly36963 requested a review from bcoe September 7, 2021 19:59
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