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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e58eb81
fix: boolean option should work with strict
jly36963 Aug 1, 2021
79c76c1
Merge branch 'master' into fix-boolean-strict-bug
jly36963 Aug 1, 2021
acbd93b
Merge branch 'master' into fix-boolean-strict-bug
jly36963 Aug 5, 2021
2b1b65d
Merge branch 'master' into fix-boolean-strict-bug
jly36963 Sep 5, 2021
48c02f1
Merge branch 'master' into fix-boolean-strict-bug
jly36963 Sep 6, 2021
0a7f641
lint
jly36963 Sep 6, 2021
f466eef
Merge branch 'master' into fix-boolean-strict-bug
jly36963 Sep 7, 2021
c356666
Merge branch 'master' into fix-boolean-strict-bug
jly36963 Sep 8, 2021
2293c3b
Merge branch 'main' into fix-boolean-strict-bug
bcoe Sep 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 20 additions & 3 deletions lib/yargs-factory.ts
View file
Open in desktop
Expand Up @@ -124,6 +124,7 @@ const kRunYargsParserAndExecuteCommands = Symbol(
);
const kRunValidation = Symbol('runValidation');
const kSetHasOutput = Symbol('setHasOutput');
const kTrackManuallySetKeys = Symbol('kTrackManuallySetKeys');
export interface YargsInternalMethods {
getCommandInstance(): CommandInstance;
getContext(): Context;
Expand Down Expand Up @@ -283,11 +284,13 @@ export class YargsInstance {
array(keys: string | string[]): YargsInstance {
argsert('<array|string>', [keys], arguments.length);
this[kPopulateParserHintArray]('array', keys);
this[kTrackManuallySetKeys](keys);
return this;
}
boolean(keys: string | string[]): YargsInstance {
argsert('<array|string>', [keys], arguments.length);
this[kPopulateParserHintArray]('boolean', keys);
this[kTrackManuallySetKeys](keys);
return this;
}
check(f: (argv: Arguments) => any, global?: boolean): YargsInstance {
Expand Down Expand Up @@ -530,6 +533,7 @@ export class YargsInstance {
count(keys: string | string[]): YargsInstance {
argsert('<array|string>', [keys], arguments.length);
this[kPopulateParserHintArray]('count', keys);
this[kTrackManuallySetKeys](keys);
return this;
}
default(
Expand Down Expand Up @@ -890,6 +894,7 @@ export class YargsInstance {
number(keys: string | string[]): YargsInstance {
argsert('<array|string>', [keys], arguments.length);
this[kPopulateParserHintArray]('number', keys);
this[kTrackManuallySetKeys](keys);
return this;
}
option(
Expand All @@ -906,6 +911,8 @@ export class YargsInstance {
opt = {};
}

this[kTrackManuallySetKeys](key);

// Warn about version name collision
// Addresses: https://github.com/yargs/yargs/issues/1979
if (this.#versionOpt && (key === 'version' || opt?.alias === 'version')) {
Expand Down Expand Up @@ -1327,9 +1334,10 @@ export class YargsInstance {
this.#strictOptions = enabled !== false;
return this;
}
string(key: string | string[]): YargsInstance {
argsert('<array|string>', [key], arguments.length);
this[kPopulateParserHintArray]('string', key);
string(keys: string | string[]): YargsInstance {
argsert('<array|string>', [keys], arguments.length);
this[kPopulateParserHintArray]('string', keys);
this[kTrackManuallySetKeys](keys);
return this;
}
terminalWidth(): number | null {
Expand Down Expand Up @@ -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: {}
}

} else {
for (const k of keys) {
this.#options.key[k] = true;
}
}
}
}

export function isYargsInstance(y: YargsInstance | void): y is YargsInstance {
Expand Down
40 changes: 40 additions & 0 deletions test/validation.cjs
View file
Open in desktop
Expand Up @@ -1021,6 +1021,46 @@ describe('validation tests', () => {
args.matey.should.equal('ben');
args._.should.deep.equal(['ahoy', '--arrr']);
});

it('does not fail when options are defined using array/boolean/count/number/string', () => {
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(() => {
expect.fail();
}),
handler: argv => {
argv.opt1.should.equal(true);
argv.opt2.should.equal(true);
argv.opt3.should.deep.equal(['foo', 'bar', 'baz']);
argv.opt4.should.equal(true);
argv.opt5.should.equal(1);
argv.opt6.should.equal(3);
argv.opt7.should.equal('cat');
},
})
.help()
.strict()
.parse(
'cmd --opt1 --opt2 --opt3 foo bar baz --opt4 --opt5 --opt6 3 --opt7 cat'
);
});
});

describe('demandOption', () => {
Expand Down