From bb1516eca8febbbc576ad5feda882b33be1d09ed Mon Sep 17 00:00:00 2001 From: erezrokah Date: Sat, 15 Jan 2022 21:32:46 +0100 Subject: [PATCH 01/20] feat(option): allow to set options as exclusive --- Readme.md | 3 ++- examples/options-extra.js | 4 ++- lib/command.js | 36 ++++++++++++++++++++++++++ lib/option.js | 13 ++++++++++ tests/command.exclusive.test.js | 45 +++++++++++++++++++++++++++++++++ typings/index.d.ts | 5 ++++ 6 files changed, 104 insertions(+), 2 deletions(-) create mode 100644 tests/command.exclusive.test.js diff --git a/Readme.md b/Readme.md index 992b3bdc7..54873d9b2 100644 --- a/Readme.md +++ b/Readme.md @@ -397,7 +397,8 @@ program .addOption(new Option('-t, --timeout ', 'timeout in seconds').default(60, 'one minute')) .addOption(new Option('-d, --drink ', 'drink size').choices(['small', 'medium', 'large'])) .addOption(new Option('-p, --port ', 'port number').env('PORT')) - .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)); + .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)) + .addOption(new Option('-d, --disable-server', 'disables the server').exclusive(['port'])); ``` ```bash diff --git a/examples/options-extra.js b/examples/options-extra.js index 62c78debe..815faa97e 100644 --- a/examples/options-extra.js +++ b/examples/options-extra.js @@ -12,7 +12,8 @@ program .addOption(new Option('-t, --timeout ', 'timeout in seconds').default(60, 'one minute')) .addOption(new Option('-d, --drink ', 'drink cup size').choices(['small', 'medium', 'large'])) .addOption(new Option('-p, --port ', 'port number').env('PORT')) - .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)); + .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)) + .addOption(new Option('-d, --disable-server', 'disables the server').exclusive(['port'])); program.parse(); @@ -24,3 +25,4 @@ console.log('Options: ', program.opts()); // PORT=80 node options-extra.js // node options-extra.js --donate // node options-extra.js --donate 30.50 +// node options-extra.js --disable-server --port 8000 diff --git a/lib/command.js b/lib/command.js index aa0d7c03d..b950c9511 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1216,6 +1216,7 @@ Expecting one of '${allowedValues.join("', '")}'`); outputHelpIfRequested(this, parsed.unknown); this._checkForMissingMandatoryOptions(); + this._checkForExclusiveOptions(); // We do not always call this check to avoid masking a "better" error, like unknown command. const checkForUnknownOptions = () => { @@ -1308,6 +1309,30 @@ Expecting one of '${allowedValues.join("', '")}'`); } } + /** + * Display an error message if exclusive options are used together. + * + * @api private + */ + _checkForExclusiveOptions() { + const definedOptions = this.options.filter( + (option) => this.getOptionValue(option.attributeName()) !== undefined + ); + + const optionsWithExclusiveNames = definedOptions.filter( + (option) => option.exclusiveNames.length > 0 + ); + + optionsWithExclusiveNames.forEach((option) => { + const exclusiveAndDefined = definedOptions.filter((defined) => + option.exclusiveNames.includes(defined.attributeName()) + ); + if (exclusiveAndDefined.length > 0) { + this.exclusiveOptionPresent(option, exclusiveAndDefined[0]); + } + }); + }; + /** * Parse options from `argv` removing known options, * and return argv split into operands and unknown arguments. @@ -1559,6 +1584,17 @@ Expecting one of '${allowedValues.join("', '")}'`); this.error(message, { code: 'commander.missingMandatoryOptionValue' }); } + /** + * `Option` conflicts with another option. + * + * @param {Option} option + * @api private + */ + exclusiveOptionPresent(option, exclusive) { + const message = `error: option '${option.flags}' cannot be used with '${exclusive.flags}'`; + this.error(message, { code:'commander.exclusiveOptionPresent' }); + }; + /** * Unknown option `flag`. * diff --git a/lib/option.js b/lib/option.js index c68110e4a..2ec8ca15c 100644 --- a/lib/option.js +++ b/lib/option.js @@ -33,6 +33,7 @@ class Option { this.parseArg = undefined; this.hidden = false; this.argChoices = undefined; + this.exclusiveNames = []; } /** @@ -66,6 +67,18 @@ class Option { return this; } + /** + * Set options names that are exclusive to this option. + * + * @param {string[]} names + * @return {Option} + */ + + exclusive(names) { + this.exclusiveNames = names; + return this; + }; + /** * Set environment variable to check for option value. * Priority order of option values is default < env < cli diff --git a/tests/command.exclusive.test.js b/tests/command.exclusive.test.js new file mode 100644 index 000000000..28aadf16b --- /dev/null +++ b/tests/command.exclusive.test.js @@ -0,0 +1,45 @@ +const commander = require('../'); + +describe('command with exclusive options', () => { + function makeProgram() { + const actionMock = jest.fn(); + const program = new commander.Command(); + program + .exitOverride() + .command('foo') + .option('-s, --silent', "Don't print anything") + .addOption( + new commander.Option('-j, --json', 'Format output as json').exclusive([ + 'silent' + ]) + ) + .action(actionMock); + + return { program, actionMock }; + } + + test('should call action if there are no explicit exlucsive options set', () => { + const { program, actionMock } = makeProgram(); + program.parse('node test.js foo --json'.split(' ')); + expect(actionMock).toHaveBeenCalledTimes(1); + expect(actionMock).toHaveBeenCalledWith({ json: true }, expect.any(Object)); + }); + + test('should call action when there are no implicit exlucsive options set', () => { + const { program, actionMock } = makeProgram(); + program.parse('node test.js foo --silent'.split(' ')); + expect(actionMock).toHaveBeenCalledTimes(1); + expect(actionMock).toHaveBeenCalledWith( + { silent: true }, + expect.any(Object) + ); + }); + + test('should exit with error if exclusive options were set', () => { + const { program } = makeProgram(); + + expect(() => { + program.parse('node test.js foo --silent --json'.split(' ')); + }).toThrow("error: option '-j, --json' cannot be used with '-s, --silent'"); + }); +}); diff --git a/typings/index.d.ts b/typings/index.d.ts index cb7bf608b..aee974fa3 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -118,6 +118,11 @@ export class Option { */ preset(arg: unknown): this; + /** + * Set options names that are exclusive to this option. + */ + exclusive(names: string[]): this; + /** * Set environment variable to check for option value. * Priority order of option values is default < env < cli From 2790a7754ad973b1a410ea3a724f5cac283a5ca5 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Mon, 31 Jan 2022 10:35:05 +0100 Subject: [PATCH 02/20] feat: add exclusive to optionDescription --- lib/help.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/help.js b/lib/help.js index eb4d2064b..847a425f0 100644 --- a/lib/help.js +++ b/lib/help.js @@ -257,6 +257,9 @@ class Help { if (option.envVar !== undefined) { extraInfo.push(`env: ${option.envVar}`); } + if (option.exclusiveNames.length > 0) { + extraInfo.push(`exclusive: ${option.exclusiveNames.join(', ')}`); + } if (extraInfo.length > 0) { return `${option.description} (${extraInfo.join(', ')})`; } From df67cdd70b4c42881358edf0c74ef5395681a4f5 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Mon, 31 Jan 2022 19:12:45 +0100 Subject: [PATCH 03/20] feat: improve error reporting --- lib/command.js | 20 +++++++++--- lib/option.js | 2 +- tests/command.exclusive.test.js | 56 ++++++++++++++++++++++++++++++--- tests/command.help.test.js | 8 +++++ 4 files changed, 76 insertions(+), 10 deletions(-) diff --git a/lib/command.js b/lib/command.js index b950c9511..59dac628e 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1331,7 +1331,7 @@ Expecting one of '${allowedValues.join("', '")}'`); this.exclusiveOptionPresent(option, exclusiveAndDefined[0]); } }); - }; + } /** * Parse options from `argv` removing known options, @@ -1591,9 +1591,21 @@ Expecting one of '${allowedValues.join("', '")}'`); * @api private */ exclusiveOptionPresent(option, exclusive) { - const message = `error: option '${option.flags}' cannot be used with '${exclusive.flags}'`; - this.error(message, { code:'commander.exclusiveOptionPresent' }); - }; + const getErrorMessage = (option) => { + const optionKey = option.attributeName(); + const source = this.getOptionValueSource(optionKey); + if (source === 'default') { + return `option '${optionKey}' with default value`; + } + if (source === 'env') { + return `environment variable '${option.envVar}'`; + } + return `option '${option.flags}'`; + }; + + const message = `error: ${getErrorMessage(option)} cannot be used with ${getErrorMessage(exclusive)}`; + this.error(message, { code: 'commander.exclusiveOptionPresent' }); + } /** * Unknown option `flag`. diff --git a/lib/option.js b/lib/option.js index 2ec8ca15c..0acd5e207 100644 --- a/lib/option.js +++ b/lib/option.js @@ -77,7 +77,7 @@ class Option { exclusive(names) { this.exclusiveNames = names; return this; - }; + } /** * Set environment variable to check for option value. diff --git a/tests/command.exclusive.test.js b/tests/command.exclusive.test.js index 28aadf16b..1709890f6 100644 --- a/tests/command.exclusive.test.js +++ b/tests/command.exclusive.test.js @@ -7,9 +7,9 @@ describe('command with exclusive options', () => { program .exitOverride() .command('foo') - .option('-s, --silent', "Don't print anything") + .addOption(new commander.Option('-s, --silent', "Don't print anything").env('SILENT')) .addOption( - new commander.Option('-j, --json', 'Format output as json').exclusive([ + new commander.Option('-j, --json', 'Format output as json').env('JSON').exclusive([ 'silent' ]) ) @@ -18,14 +18,19 @@ describe('command with exclusive options', () => { return { program, actionMock }; } - test('should call action if there are no explicit exlucsive options set', () => { + beforeEach(() => { + delete process.env.SILENT; + delete process.env.JSON; + }); + + test('should call action if there are no explicit exclusive options set', () => { const { program, actionMock } = makeProgram(); program.parse('node test.js foo --json'.split(' ')); expect(actionMock).toHaveBeenCalledTimes(1); expect(actionMock).toHaveBeenCalledWith({ json: true }, expect.any(Object)); }); - test('should call action when there are no implicit exlucsive options set', () => { + test('should call action when there are no implicit exclusive options set', () => { const { program, actionMock } = makeProgram(); program.parse('node test.js foo --silent'.split(' ')); expect(actionMock).toHaveBeenCalledTimes(1); @@ -40,6 +45,47 @@ describe('command with exclusive options', () => { expect(() => { program.parse('node test.js foo --silent --json'.split(' ')); - }).toThrow("error: option '-j, --json' cannot be used with '-s, --silent'"); + }).toThrow("error: option '-j, --json' cannot be used with option '-s, --silent'"); + }); + + test('should report the env variable as the exclusive option source, when exclusive option is set', () => { + const { program } = makeProgram(); + + process.env.SILENT = true; + + expect(() => { + program.parse('node test.js foo --json'.split(' ')); + }).toThrow("error: option '-j, --json' cannot be used with environment variable 'SILENT'"); + }); + + test('should report the env variable as the configured option source, when configured option is set', () => { + const { program } = makeProgram(); + + process.env.JSON = true; + + expect(() => { + program.parse('node test.js foo --silent'.split(' ')); + }).toThrow("error: environment variable 'JSON' cannot be used with option '-s, --silent'"); + }); + + test('should report both env variables as sources, when configured option and exclusive option are set', () => { + const { program } = makeProgram(); + + process.env.SILENT = true; + process.env.JSON = true; + + expect(() => { + program.parse('node test.js foo'.split(' ')); + }).toThrow("error: environment variable 'JSON' cannot be used with environment variable 'SILENT'"); + }); + + test('should exit with error if default value is conflicting', () => { + const { program } = makeProgram(); + + program.commands[0].addOption(new commander.Option('-d, --debug', 'print debug logs').default(true).exclusive('silent')); + + expect(() => { + program.parse('node test.js foo --silent'.split(' ')); + }).toThrow("error: option 'debug' with default value cannot be used with option '-s, --silent'"); }); }); diff --git a/tests/command.help.test.js b/tests/command.help.test.js index c7745bfd7..57175c6d7 100644 --- a/tests/command.help.test.js +++ b/tests/command.help.test.js @@ -297,3 +297,11 @@ test('when argument has choices and default then both included in helpInformatio const helpInformation = program.helpInformation(); expect(helpInformation).toMatch('(choices: "red", "blue", default: "red")'); }); + +test('when option has exclusive names they are included in the helpInformation', () => { + const program = new commander.Command(); + program + .addOption(new commander.Option('-p, --port ').exclusive(['disable-server', 'use-default-port'])); + const helpInformation = program.helpInformation(); + expect(helpInformation).toMatch('(exclusive: disable-server, use-default-port)'); +}); From d80e814b6a34cce48f52395d13c7b2fa2a26e455 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Sat, 5 Feb 2022 21:59:20 +0100 Subject: [PATCH 04/20] fix: remove from help --- lib/help.js | 3 --- tests/command.help.test.js | 8 -------- 2 files changed, 11 deletions(-) diff --git a/lib/help.js b/lib/help.js index 847a425f0..eb4d2064b 100644 --- a/lib/help.js +++ b/lib/help.js @@ -257,9 +257,6 @@ class Help { if (option.envVar !== undefined) { extraInfo.push(`env: ${option.envVar}`); } - if (option.exclusiveNames.length > 0) { - extraInfo.push(`exclusive: ${option.exclusiveNames.join(', ')}`); - } if (extraInfo.length > 0) { return `${option.description} (${extraInfo.join(', ')})`; } diff --git a/tests/command.help.test.js b/tests/command.help.test.js index 57175c6d7..c7745bfd7 100644 --- a/tests/command.help.test.js +++ b/tests/command.help.test.js @@ -297,11 +297,3 @@ test('when argument has choices and default then both included in helpInformatio const helpInformation = program.helpInformation(); expect(helpInformation).toMatch('(choices: "red", "blue", default: "red")'); }); - -test('when option has exclusive names they are included in the helpInformation', () => { - const program = new commander.Command(); - program - .addOption(new commander.Option('-p, --port ').exclusive(['disable-server', 'use-default-port'])); - const helpInformation = program.helpInformation(); - expect(helpInformation).toMatch('(exclusive: disable-server, use-default-port)'); -}); From 92736388be97e706eb5342d26cfa03875a0ea6bc Mon Sep 17 00:00:00 2001 From: erezrokah Date: Mon, 7 Feb 2022 10:18:20 +0100 Subject: [PATCH 05/20] fix: don't count default values as exclusive --- lib/command.js | 17 ++++++++++------- tests/command.exclusive.test.js | 11 ++++++----- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/lib/command.js b/lib/command.js index 59dac628e..dc92d536e 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1315,16 +1315,22 @@ Expecting one of '${allowedValues.join("', '")}'`); * @api private */ _checkForExclusiveOptions() { - const definedOptions = this.options.filter( - (option) => this.getOptionValue(option.attributeName()) !== undefined + const definedNonDefaultOptions = this.options.filter( + (option) => { + const optionKey = option.attributeName(); + if (this.getOptionValue(optionKey) === undefined) { + return false; + } + return this.getOptionValueSource(optionKey) !== 'default'; + } ); - const optionsWithExclusiveNames = definedOptions.filter( + const optionsWithExclusiveNames = definedNonDefaultOptions.filter( (option) => option.exclusiveNames.length > 0 ); optionsWithExclusiveNames.forEach((option) => { - const exclusiveAndDefined = definedOptions.filter((defined) => + const exclusiveAndDefined = definedNonDefaultOptions.filter((defined) => option.exclusiveNames.includes(defined.attributeName()) ); if (exclusiveAndDefined.length > 0) { @@ -1594,9 +1600,6 @@ Expecting one of '${allowedValues.join("', '")}'`); const getErrorMessage = (option) => { const optionKey = option.attributeName(); const source = this.getOptionValueSource(optionKey); - if (source === 'default') { - return `option '${optionKey}' with default value`; - } if (source === 'env') { return `environment variable '${option.envVar}'`; } diff --git a/tests/command.exclusive.test.js b/tests/command.exclusive.test.js index 1709890f6..9adc0fde5 100644 --- a/tests/command.exclusive.test.js +++ b/tests/command.exclusive.test.js @@ -79,13 +79,14 @@ describe('command with exclusive options', () => { }).toThrow("error: environment variable 'JSON' cannot be used with environment variable 'SILENT'"); }); - test('should exit with error if default value is conflicting', () => { - const { program } = makeProgram(); + test('should allow default value with exclusive option', () => { + const { program, actionMock } = makeProgram(); program.commands[0].addOption(new commander.Option('-d, --debug', 'print debug logs').default(true).exclusive('silent')); - expect(() => { - program.parse('node test.js foo --silent'.split(' ')); - }).toThrow("error: option 'debug' with default value cannot be used with option '-s, --silent'"); + program.parse('node test.js foo --silent'.split(' ')); + + expect(actionMock).toHaveBeenCalledTimes(1); + expect(actionMock).toHaveBeenCalledWith({ debug: true, silent: true }, expect.any(Object)); }); }); From adabd8750161322c38101cbfcfaf2ef3d8aa5026 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Thu, 10 Feb 2022 09:55:32 +0100 Subject: [PATCH 06/20] refactor: prefix exclusiveOptionPresent with _ --- lib/command.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/command.js b/lib/command.js index dc92d536e..13f6e4821 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1334,7 +1334,7 @@ Expecting one of '${allowedValues.join("', '")}'`); option.exclusiveNames.includes(defined.attributeName()) ); if (exclusiveAndDefined.length > 0) { - this.exclusiveOptionPresent(option, exclusiveAndDefined[0]); + this._exclusiveOptionPresent(option, exclusiveAndDefined[0]); } }); } @@ -1596,7 +1596,7 @@ Expecting one of '${allowedValues.join("', '")}'`); * @param {Option} option * @api private */ - exclusiveOptionPresent(option, exclusive) { + _exclusiveOptionPresent(option, exclusive) { const getErrorMessage = (option) => { const optionKey = option.attributeName(); const source = this.getOptionValueSource(optionKey); From 744a3396086477867d889a48eb13c41837e8a2f3 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Thu, 10 Feb 2022 09:57:03 +0100 Subject: [PATCH 07/20] refactor: use find instead of filter --- lib/command.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/command.js b/lib/command.js index 13f6e4821..3035fa811 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1330,11 +1330,11 @@ Expecting one of '${allowedValues.join("', '")}'`); ); optionsWithExclusiveNames.forEach((option) => { - const exclusiveAndDefined = definedNonDefaultOptions.filter((defined) => + const exclusiveAndDefined = definedNonDefaultOptions.find((defined) => option.exclusiveNames.includes(defined.attributeName()) ); - if (exclusiveAndDefined.length > 0) { - this._exclusiveOptionPresent(option, exclusiveAndDefined[0]); + if (exclusiveAndDefined) { + this._exclusiveOptionPresent(option, exclusiveAndDefined); } }); } From 249a474996adacbbe702bdb49b04a14aaefe1314 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Thu, 10 Feb 2022 10:27:36 +0100 Subject: [PATCH 08/20] feat: rename exclusive to conflicts --- Readme.md | 2 +- examples/options-extra.js | 2 +- lib/command.js | 27 ++++++++++--------- lib/option.js | 8 +++--- ...sive.test.js => command.conflicts.test.js} | 18 ++++++------- typings/index.d.ts | 4 +-- 6 files changed, 31 insertions(+), 30 deletions(-) rename tests/{command.exclusive.test.js => command.conflicts.test.js} (81%) diff --git a/Readme.md b/Readme.md index 54873d9b2..23f133676 100644 --- a/Readme.md +++ b/Readme.md @@ -398,7 +398,7 @@ program .addOption(new Option('-d, --drink ', 'drink size').choices(['small', 'medium', 'large'])) .addOption(new Option('-p, --port ', 'port number').env('PORT')) .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)) - .addOption(new Option('-d, --disable-server', 'disables the server').exclusive(['port'])); + .addOption(new Option('-d, --disable-server', 'disables the server').conflicts(['port'])); ``` ```bash diff --git a/examples/options-extra.js b/examples/options-extra.js index 815faa97e..71bb95d93 100644 --- a/examples/options-extra.js +++ b/examples/options-extra.js @@ -13,7 +13,7 @@ program .addOption(new Option('-d, --drink ', 'drink cup size').choices(['small', 'medium', 'large'])) .addOption(new Option('-p, --port ', 'port number').env('PORT')) .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)) - .addOption(new Option('-d, --disable-server', 'disables the server').exclusive(['port'])); + .addOption(new Option('-d, --disable-server', 'disables the server').conflicts(['port'])); program.parse(); diff --git a/lib/command.js b/lib/command.js index 3035fa811..dc23774b0 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1216,7 +1216,7 @@ Expecting one of '${allowedValues.join("', '")}'`); outputHelpIfRequested(this, parsed.unknown); this._checkForMissingMandatoryOptions(); - this._checkForExclusiveOptions(); + this._checkForConflictingOptions(); // We do not always call this check to avoid masking a "better" error, like unknown command. const checkForUnknownOptions = () => { @@ -1310,11 +1310,11 @@ Expecting one of '${allowedValues.join("', '")}'`); } /** - * Display an error message if exclusive options are used together. + * Display an error message if conflicting options are used together. * * @api private */ - _checkForExclusiveOptions() { + _checkForConflictingOptions() { const definedNonDefaultOptions = this.options.filter( (option) => { const optionKey = option.attributeName(); @@ -1325,16 +1325,16 @@ Expecting one of '${allowedValues.join("', '")}'`); } ); - const optionsWithExclusiveNames = definedNonDefaultOptions.filter( - (option) => option.exclusiveNames.length > 0 + const optionsWithConflicting = definedNonDefaultOptions.filter( + (option) => option.conflictsWith.length > 0 ); - optionsWithExclusiveNames.forEach((option) => { - const exclusiveAndDefined = definedNonDefaultOptions.find((defined) => - option.exclusiveNames.includes(defined.attributeName()) + optionsWithConflicting.forEach((option) => { + const conflictingAndDefined = definedNonDefaultOptions.find((defined) => + option.conflictsWith.includes(defined.attributeName()) ); - if (exclusiveAndDefined) { - this._exclusiveOptionPresent(option, exclusiveAndDefined); + if (conflictingAndDefined) { + this._conflictingOptionPresent(option, conflictingAndDefined); } }); } @@ -1594,9 +1594,10 @@ Expecting one of '${allowedValues.join("', '")}'`); * `Option` conflicts with another option. * * @param {Option} option + * @param {Option} conflictingOption * @api private */ - _exclusiveOptionPresent(option, exclusive) { + _conflictingOptionPresent(option, conflictingOption) { const getErrorMessage = (option) => { const optionKey = option.attributeName(); const source = this.getOptionValueSource(optionKey); @@ -1606,8 +1607,8 @@ Expecting one of '${allowedValues.join("', '")}'`); return `option '${option.flags}'`; }; - const message = `error: ${getErrorMessage(option)} cannot be used with ${getErrorMessage(exclusive)}`; - this.error(message, { code: 'commander.exclusiveOptionPresent' }); + const message = `error: ${getErrorMessage(option)} cannot be used with ${getErrorMessage(conflictingOption)}`; + this.error(message, { code: 'commander.conflictingOptionPresent' }); } /** diff --git a/lib/option.js b/lib/option.js index 0acd5e207..42159c745 100644 --- a/lib/option.js +++ b/lib/option.js @@ -33,7 +33,7 @@ class Option { this.parseArg = undefined; this.hidden = false; this.argChoices = undefined; - this.exclusiveNames = []; + this.conflictsWith = []; } /** @@ -68,14 +68,14 @@ class Option { } /** - * Set options names that are exclusive to this option. + * Set options names that are conflicts with this option. * * @param {string[]} names * @return {Option} */ - exclusive(names) { - this.exclusiveNames = names; + conflicts(names) { + this.conflictsWith = names; return this; } diff --git a/tests/command.exclusive.test.js b/tests/command.conflicts.test.js similarity index 81% rename from tests/command.exclusive.test.js rename to tests/command.conflicts.test.js index 9adc0fde5..0b8c777e8 100644 --- a/tests/command.exclusive.test.js +++ b/tests/command.conflicts.test.js @@ -1,6 +1,6 @@ const commander = require('../'); -describe('command with exclusive options', () => { +describe('command with conflicting options', () => { function makeProgram() { const actionMock = jest.fn(); const program = new commander.Command(); @@ -9,7 +9,7 @@ describe('command with exclusive options', () => { .command('foo') .addOption(new commander.Option('-s, --silent', "Don't print anything").env('SILENT')) .addOption( - new commander.Option('-j, --json', 'Format output as json').env('JSON').exclusive([ + new commander.Option('-j, --json', 'Format output as json').env('JSON').conflicts([ 'silent' ]) ) @@ -23,14 +23,14 @@ describe('command with exclusive options', () => { delete process.env.JSON; }); - test('should call action if there are no explicit exclusive options set', () => { + test('should call action if there are no explicit conflicting options set', () => { const { program, actionMock } = makeProgram(); program.parse('node test.js foo --json'.split(' ')); expect(actionMock).toHaveBeenCalledTimes(1); expect(actionMock).toHaveBeenCalledWith({ json: true }, expect.any(Object)); }); - test('should call action when there are no implicit exclusive options set', () => { + test('should call action when there are no implicit conflicting options set', () => { const { program, actionMock } = makeProgram(); program.parse('node test.js foo --silent'.split(' ')); expect(actionMock).toHaveBeenCalledTimes(1); @@ -40,7 +40,7 @@ describe('command with exclusive options', () => { ); }); - test('should exit with error if exclusive options were set', () => { + test('should exit with error if conflicting options were set', () => { const { program } = makeProgram(); expect(() => { @@ -48,7 +48,7 @@ describe('command with exclusive options', () => { }).toThrow("error: option '-j, --json' cannot be used with option '-s, --silent'"); }); - test('should report the env variable as the exclusive option source, when exclusive option is set', () => { + test('should report the env variable as the conflicting option source, when conflicting option is set', () => { const { program } = makeProgram(); process.env.SILENT = true; @@ -68,7 +68,7 @@ describe('command with exclusive options', () => { }).toThrow("error: environment variable 'JSON' cannot be used with option '-s, --silent'"); }); - test('should report both env variables as sources, when configured option and exclusive option are set', () => { + test('should report both env variables as sources, when configured option and conflicting option are set', () => { const { program } = makeProgram(); process.env.SILENT = true; @@ -79,10 +79,10 @@ describe('command with exclusive options', () => { }).toThrow("error: environment variable 'JSON' cannot be used with environment variable 'SILENT'"); }); - test('should allow default value with exclusive option', () => { + test('should allow default value with a conflicting option', () => { const { program, actionMock } = makeProgram(); - program.commands[0].addOption(new commander.Option('-d, --debug', 'print debug logs').default(true).exclusive('silent')); + program.commands[0].addOption(new commander.Option('-d, --debug', 'print debug logs').default(true).conflicts(['silent'])); program.parse('node test.js foo --silent'.split(' ')); diff --git a/typings/index.d.ts b/typings/index.d.ts index aee974fa3..1f1689b07 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -119,9 +119,9 @@ export class Option { preset(arg: unknown): this; /** - * Set options names that are exclusive to this option. + * Set options names that conflict with this option. */ - exclusive(names: string[]): this; + conflicts(names: string[]): this; /** * Set environment variable to check for option value. From e699665ccb0fda9f67bfa61c0d0a46a69777520e Mon Sep 17 00:00:00 2001 From: erezrokah Date: Thu, 10 Feb 2022 10:45:13 +0100 Subject: [PATCH 09/20] test: add typing test --- typings/index.test-d.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/typings/index.test-d.ts b/typings/index.test-d.ts index a5536e542..9da76b62b 100644 --- a/typings/index.test-d.ts +++ b/typings/index.test-d.ts @@ -414,6 +414,9 @@ expectType(baseOption.hideHelp(false)); expectType(baseOption.choices(['a', 'b'])); expectType(baseOption.choices(['a', 'b'] as const)); +// conflicts +expectType(baseOption.conflicts(['a', 'b'])); + // name expectType(baseOption.name()); From 9d641d4d0e70926a7e9b5af4593b2a8b26bb530a Mon Sep 17 00:00:00 2001 From: erezrokah Date: Thu, 10 Feb 2022 11:07:42 +0100 Subject: [PATCH 10/20] test: add chaining test --- tests/option.chain.test.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/option.chain.test.js b/tests/option.chain.test.js index fd9d826ee..3ed59688d 100644 --- a/tests/option.chain.test.js +++ b/tests/option.chain.test.js @@ -36,4 +36,10 @@ describe('Option methods that should return this for chaining', () => { const result = option.env('e'); expect(result).toBe(option); }); + + test('when call .conflicts() then returns this', () => { + const option = new Option('-e,--example '); + const result = option.conflicts(['a']); + expect(result).toBe(option); + }); }); From 81065dc6f0d8e1fcdf6b10a3601372c73c1984fe Mon Sep 17 00:00:00 2001 From: erezrokah Date: Fri, 11 Feb 2022 09:28:07 +0100 Subject: [PATCH 11/20] test: add error code valdiation test --- tests/command.exitOverride.test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/command.exitOverride.test.js b/tests/command.exitOverride.test.js index 09b7b4989..d99d6beca 100644 --- a/tests/command.exitOverride.test.js +++ b/tests/command.exitOverride.test.js @@ -332,6 +332,23 @@ describe('.exitOverride and error details', () => { expectCommanderError(caughtErr, 1, 'commander.invalidArgument', "error: command-argument value 'green' is invalid for argument 'n'. NO"); }); + test('when has conflicting option then throw CommanderError', () => { + const program = new commander.Command(); + program + .exitOverride() + .addOption(new commander.Option('--silent')) + .addOption(new commander.Option('--debug').conflicts(['silent'])); + + let caughtErr; + try { + program.parse(['--debug', '--silent'], { from: 'user' }); + } catch (err) { + caughtErr = err; + } + + expectCommanderError(caughtErr, 1, 'commander.conflictingOptionPresent', "error: option '--debug' cannot be used with option '--silent'"); + }); + test('when call error() then throw CommanderError', () => { const program = new commander.Command(); program From f7739debcc830d3017c98d4cbfb797814d1e815a Mon Sep 17 00:00:00 2001 From: erezrokah Date: Sat, 12 Feb 2022 09:35:04 +0100 Subject: [PATCH 12/20] refactor: drop present from method and error --- lib/command.js | 6 +++--- tests/command.exitOverride.test.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/command.js b/lib/command.js index dc23774b0..0bc48ed20 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1334,7 +1334,7 @@ Expecting one of '${allowedValues.join("', '")}'`); option.conflictsWith.includes(defined.attributeName()) ); if (conflictingAndDefined) { - this._conflictingOptionPresent(option, conflictingAndDefined); + this._conflictingOption(option, conflictingAndDefined); } }); } @@ -1597,7 +1597,7 @@ Expecting one of '${allowedValues.join("', '")}'`); * @param {Option} conflictingOption * @api private */ - _conflictingOptionPresent(option, conflictingOption) { + _conflictingOption(option, conflictingOption) { const getErrorMessage = (option) => { const optionKey = option.attributeName(); const source = this.getOptionValueSource(optionKey); @@ -1608,7 +1608,7 @@ Expecting one of '${allowedValues.join("', '")}'`); }; const message = `error: ${getErrorMessage(option)} cannot be used with ${getErrorMessage(conflictingOption)}`; - this.error(message, { code: 'commander.conflictingOptionPresent' }); + this.error(message, { code: 'commander.conflictingOption' }); } /** diff --git a/tests/command.exitOverride.test.js b/tests/command.exitOverride.test.js index d99d6beca..08614e180 100644 --- a/tests/command.exitOverride.test.js +++ b/tests/command.exitOverride.test.js @@ -346,7 +346,7 @@ describe('.exitOverride and error details', () => { caughtErr = err; } - expectCommanderError(caughtErr, 1, 'commander.conflictingOptionPresent', "error: option '--debug' cannot be used with option '--silent'"); + expectCommanderError(caughtErr, 1, 'commander.conflictingOption', "error: option '--debug' cannot be used with option '--silent'"); }); test('when call error() then throw CommanderError', () => { From 351ef38144c0eae579ba2365b011117d3082067e Mon Sep 17 00:00:00 2001 From: erezrokah Date: Sun, 20 Feb 2022 10:21:55 +0100 Subject: [PATCH 13/20] test: suppress errors in tests --- tests/command.conflicts.test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/command.conflicts.test.js b/tests/command.conflicts.test.js index 0b8c777e8..a1e55d103 100644 --- a/tests/command.conflicts.test.js +++ b/tests/command.conflicts.test.js @@ -6,6 +6,9 @@ describe('command with conflicting options', () => { const program = new commander.Command(); program .exitOverride() + .configureOutput({ + writeErr: () => {} + }) .command('foo') .addOption(new commander.Option('-s, --silent', "Don't print anything").env('SILENT')) .addOption( From 91ab6b11c0cababe6c6fbf3354693c126c6625fd Mon Sep 17 00:00:00 2001 From: erezrokah Date: Sun, 20 Feb 2022 10:25:01 +0100 Subject: [PATCH 14/20] fix(docs): add missing option in help --- Readme.md | 1 + 1 file changed, 1 insertion(+) diff --git a/Readme.md b/Readme.md index 23f133676..fa826a8ad 100644 --- a/Readme.md +++ b/Readme.md @@ -410,6 +410,7 @@ Options: -d, --drink drink cup size (choices: "small", "medium", "large") -p, --port port number (env: PORT) --donate [amount] optional donation in dollars (preset: 20) + -d, --disable-server disables the server -h, --help display help for command $ extra --drink huge From 39eba86e2ac58ad22661482a12c76f0d2d4ab808 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Sun, 20 Feb 2022 20:44:01 +0100 Subject: [PATCH 15/20] feat: handle negated option errors --- lib/command.js | 13 ++++++++++++- tests/command.conflicts.test.js | 31 +++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/lib/command.js b/lib/command.js index 0bc48ed20..0c78b94b9 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1598,6 +1598,14 @@ Expecting one of '${allowedValues.join("', '")}'`); * @api private */ _conflictingOption(option, conflictingOption) { + const getNegatedOption = (option) => { + const optionKey = option.attributeName(); + const negatedLongFlag = option.long && `--no-${optionKey}`; + const negatedOption = negatedLongFlag && this._findOption(negatedLongFlag); + + return negatedOption; + }; + const getErrorMessage = (option) => { const optionKey = option.attributeName(); const source = this.getOptionValueSource(optionKey); @@ -1607,7 +1615,10 @@ Expecting one of '${allowedValues.join("', '")}'`); return `option '${option.flags}'`; }; - const message = `error: ${getErrorMessage(option)} cannot be used with ${getErrorMessage(conflictingOption)}`; + const originOption = getNegatedOption(option) || option; + const originConflictingOption = getNegatedOption(conflictingOption) || conflictingOption; + + const message = `error: ${getErrorMessage(originOption)} cannot be used with ${getErrorMessage(originConflictingOption)}`; this.error(message, { code: 'commander.conflictingOption' }); } diff --git a/tests/command.conflicts.test.js b/tests/command.conflicts.test.js index a1e55d103..e6ea10d63 100644 --- a/tests/command.conflicts.test.js +++ b/tests/command.conflicts.test.js @@ -24,6 +24,7 @@ describe('command with conflicting options', () => { beforeEach(() => { delete process.env.SILENT; delete process.env.JSON; + delete process.env.NO_COLOR; }); test('should call action if there are no explicit conflicting options set', () => { @@ -92,4 +93,34 @@ describe('command with conflicting options', () => { expect(actionMock).toHaveBeenCalledTimes(1); expect(actionMock).toHaveBeenCalledWith({ debug: true, silent: true }, expect.any(Object)); }); + + test('should report conflict on negated option flag', () => { + const { program } = makeProgram(); + + program + .command('bar') + .addOption(new commander.Option('--red').conflicts(['color'])) + .addOption(new commander.Option('--color')) + .addOption(new commander.Option('-N, --no-color')); + + expect(() => { + program.parse('node test.js bar --red -N'.split(' ')); + }).toThrow("error: option '--red' cannot be used with option '-N, --no-color'"); + }); + + test('should report conflict on negated option env variable', () => { + const { program } = makeProgram(); + + process.env.NO_COLOR = true; + + program + .command('bar') + .addOption(new commander.Option('--red').conflicts(['color'])) + .addOption(new commander.Option('--color')) + .addOption(new commander.Option('-N, --no-color').env('NO_COLOR')); + + expect(() => { + program.parse('node test.js bar --red'.split(' ')); + }).toThrow("error: option '--red' cannot be used with environment variable 'NO_COLOR'"); + }); }); From 79e6a7cec30712bf48bab0ad56d3ec2652ca7369 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Sun, 20 Feb 2022 20:59:26 +0100 Subject: [PATCH 16/20] docs(readme): add conflicts example --- Readme.md | 7 +++++-- examples/options-extra.js | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Readme.md b/Readme.md index fa826a8ad..c9b430113 100644 --- a/Readme.md +++ b/Readme.md @@ -398,7 +398,7 @@ program .addOption(new Option('-d, --drink ', 'drink size').choices(['small', 'medium', 'large'])) .addOption(new Option('-p, --port ', 'port number').env('PORT')) .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)) - .addOption(new Option('-d, --disable-server', 'disables the server').conflicts(['port'])); + .addOption(new Option('--disable-server', 'disables the server').conflicts(['port'])); ``` ```bash @@ -410,7 +410,7 @@ Options: -d, --drink drink cup size (choices: "small", "medium", "large") -p, --port port number (env: PORT) --donate [amount] optional donation in dollars (preset: 20) - -d, --disable-server disables the server + --disable-server disables the server -h, --help display help for command $ extra --drink huge @@ -418,6 +418,9 @@ error: option '-d, --drink ' argument 'huge' is invalid. Allowed choices a $ PORT=80 extra --donate Options: { timeout: 60, donate: 20, port: '80' } + +$ extra --disable-server --port 8000 +error: option '--disable-server' cannot be used with option '-p, --port ' ``` ### Custom option processing diff --git a/examples/options-extra.js b/examples/options-extra.js index 71bb95d93..2c08092f7 100644 --- a/examples/options-extra.js +++ b/examples/options-extra.js @@ -13,7 +13,7 @@ program .addOption(new Option('-d, --drink ', 'drink cup size').choices(['small', 'medium', 'large'])) .addOption(new Option('-p, --port ', 'port number').env('PORT')) .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)) - .addOption(new Option('-d, --disable-server', 'disables the server').conflicts(['port'])); + .addOption(new Option('--disable-server', 'disables the server').conflicts(['port'])); program.parse(); From 46bf4b03aa10479473937a2ed42bdd57bdfb00ee Mon Sep 17 00:00:00 2001 From: erezrokah Date: Mon, 21 Feb 2022 11:14:50 +0100 Subject: [PATCH 17/20] fix: better handle negated options --- lib/command.js | 30 +++++---- tests/command.conflicts.test.js | 106 +++++++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 13 deletions(-) diff --git a/lib/command.js b/lib/command.js index 0c78b94b9..32d0ee2d2 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1598,27 +1598,33 @@ Expecting one of '${allowedValues.join("', '")}'`); * @api private */ _conflictingOption(option, conflictingOption) { - const getNegatedOption = (option) => { + // The calling code does not know whether a negated option is the source of the + // value, so do some work to take an educated guess. + const findBestOptionFromValue = (option) => { const optionKey = option.attributeName(); - const negatedLongFlag = option.long && `--no-${optionKey}`; - const negatedOption = negatedLongFlag && this._findOption(negatedLongFlag); - - return negatedOption; + const optionValue = this.getOptionValue(optionKey); + const negativeOption = this.options.find(target => target.negate && optionKey === target.attributeName()); + const positiveOption = this.options.find(target => !target.negate && optionKey === target.attributeName()); + if (negativeOption && ( + (negativeOption.presetArg === undefined && optionValue === false) || + (negativeOption.presetArg !== undefined && optionValue === negativeOption.presetArg) + )) { + return negativeOption; + } + return positiveOption || option; }; const getErrorMessage = (option) => { - const optionKey = option.attributeName(); + const bestOption = findBestOptionFromValue(option); + const optionKey = bestOption.attributeName(); const source = this.getOptionValueSource(optionKey); if (source === 'env') { - return `environment variable '${option.envVar}'`; + return `environment variable '${bestOption.envVar}'`; } - return `option '${option.flags}'`; + return `option '${bestOption.flags}'`; }; - const originOption = getNegatedOption(option) || option; - const originConflictingOption = getNegatedOption(conflictingOption) || conflictingOption; - - const message = `error: ${getErrorMessage(originOption)} cannot be used with ${getErrorMessage(originConflictingOption)}`; + const message = `error: ${getErrorMessage(option)} cannot be used with ${getErrorMessage(conflictingOption)}`; this.error(message, { code: 'commander.conflictingOption' }); } diff --git a/tests/command.conflicts.test.js b/tests/command.conflicts.test.js index e6ea10d63..3ec03b878 100644 --- a/tests/command.conflicts.test.js +++ b/tests/command.conflicts.test.js @@ -24,7 +24,8 @@ describe('command with conflicting options', () => { beforeEach(() => { delete process.env.SILENT; delete process.env.JSON; - delete process.env.NO_COLOR; + delete process.env.DUAL; + delete process.env.NO_DUAL; }); test('should call action if there are no explicit conflicting options set', () => { @@ -123,4 +124,107 @@ describe('command with conflicting options', () => { program.parse('node test.js bar --red'.split(' ')); }).toThrow("error: option '--red' cannot be used with environment variable 'NO_COLOR'"); }); + + test('should report correct error for shorthand negated option', () => { + const { program } = makeProgram(); + + program + .command('bar') + .addOption(new commander.Option('--red')) + .addOption(new commander.Option('-N, --no-color').conflicts(['red'])); + + expect(() => { + program.parse('node test.js bar --red -N'.split(' ')); + }).toThrow("error: option '-N, --no-color' cannot be used with option '--red'"); + }); + + test('should report correct error for positive option when negated is configured', () => { + const { program } = makeProgram(); + + program + .command('bar') + .addOption(new commander.Option('--red')) + .addOption(new commander.Option('--dual').env('DUAL').conflicts('red')) + .addOption(new commander.Option('--no-dual').env('NO_DUAL')); + + expect(() => { + program.parse('node test.js bar --red --dual'.split(' ')); + }).toThrow("error: option '--dual' cannot be used with option '--red'"); + }); + + test('should report correct error for negated option when positive is configured', () => { + const { program } = makeProgram(); + + program + .command('bar') + .addOption(new commander.Option('--red')) + .addOption(new commander.Option('--dual').env('DUAL').conflicts('red')) + .addOption(new commander.Option('--no-dual').env('NO_DUAL')); + + expect(() => { + program.parse('node test.js bar --red --no-dual'.split(' ')); + }).toThrow("error: option '--no-dual' cannot be used with option '--red'"); + }); + + test('should report correct error for positive env variable when negated is configured', () => { + const { program } = makeProgram(); + + program + .command('bar') + .addOption(new commander.Option('--red')) + .addOption(new commander.Option('--dual').env('DUAL').conflicts('red')) + .addOption(new commander.Option('--no-dual').env('NO_DUAL')); + + process.env.DUAL = 'true'; + expect(() => { + program.parse('node test.js bar --red'.split(' ')); + }).toThrow("error: environment variable 'DUAL' cannot be used with option '--red'"); + }); + + test('should report correct error for negated env variable when positive is configured', () => { + const { program } = makeProgram(); + + program + .command('bar') + .addOption(new commander.Option('--red')) + .addOption(new commander.Option('--dual').env('DUAL').conflicts('red')) + .addOption(new commander.Option('--no-dual').env('NO_DUAL')); + + process.env.NO_DUAL = 'true'; + expect(() => { + program.parse('node test.js bar --red'.split(' ')); + }).toThrow("error: environment variable 'NO_DUAL' cannot be used with option '--red'"); + }); + + test('should report correct error for positive option with string value when negated is configured', () => { + const { program } = makeProgram(); + + program + .command('bar') + .addOption(new commander.Option('--red')) + .addOption(new commander.Option('--dual2 ').conflicts('red')) + .addOption(new commander.Option('--no-dual2').preset('BAD')); + + expect(() => { + program.parse('node test.js bar --red --dual2 foo'.split(' ')); + }).toThrow("error: option '--dual2 ' cannot be used with option '--red'"); + + // expect(() => { + // program.parse('node test.js bar --red --no-dual2'.split(' ')); + // }).toThrow("error: option '--no-dual2' cannot be used with option '--red'"); + }); + + test('should report correct error for negated option with preset when negated is configured', () => { + const { program } = makeProgram(); + + program + .command('bar') + .addOption(new commander.Option('--red')) + .addOption(new commander.Option('--dual2 ').conflicts('red')) + .addOption(new commander.Option('--no-dual2').preset('BAD')); + + expect(() => { + program.parse('node test.js bar --red --no-dual2'.split(' ')); + }).toThrow("error: option '--no-dual2' cannot be used with option '--red'"); + }); }); From c67469be5dda42818f0da36794493126aebb2682 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Mon, 21 Feb 2022 21:47:54 +0100 Subject: [PATCH 18/20] test: remove comment --- tests/command.conflicts.test.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/command.conflicts.test.js b/tests/command.conflicts.test.js index 3ec03b878..4c14df3b5 100644 --- a/tests/command.conflicts.test.js +++ b/tests/command.conflicts.test.js @@ -208,10 +208,6 @@ describe('command with conflicting options', () => { expect(() => { program.parse('node test.js bar --red --dual2 foo'.split(' ')); }).toThrow("error: option '--dual2 ' cannot be used with option '--red'"); - - // expect(() => { - // program.parse('node test.js bar --red --no-dual2'.split(' ')); - // }).toThrow("error: option '--no-dual2' cannot be used with option '--red'"); }); test('should report correct error for negated option with preset when negated is configured', () => { From 0059715b6a9d501a038719f2c40a1151d012c658 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Mon, 21 Feb 2022 22:47:18 +0100 Subject: [PATCH 19/20] feat: support single string argument --- Readme.md | 2 +- examples/options-extra.js | 2 +- lib/option.js | 10 +++++--- tests/command.conflicts.test.js | 42 ++++++++++++++++++++++++++++----- typings/index.d.ts | 4 ++-- typings/index.test-d.ts | 1 + 6 files changed, 48 insertions(+), 13 deletions(-) diff --git a/Readme.md b/Readme.md index c9b430113..7743cad7b 100644 --- a/Readme.md +++ b/Readme.md @@ -398,7 +398,7 @@ program .addOption(new Option('-d, --drink ', 'drink size').choices(['small', 'medium', 'large'])) .addOption(new Option('-p, --port ', 'port number').env('PORT')) .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)) - .addOption(new Option('--disable-server', 'disables the server').conflicts(['port'])); + .addOption(new Option('--disable-server', 'disables the server').conflicts(['port'])); // or `.conflicts('port')` for a single conflict ``` ```bash diff --git a/examples/options-extra.js b/examples/options-extra.js index 2c08092f7..4f7a82390 100644 --- a/examples/options-extra.js +++ b/examples/options-extra.js @@ -13,7 +13,7 @@ program .addOption(new Option('-d, --drink ', 'drink cup size').choices(['small', 'medium', 'large'])) .addOption(new Option('-p, --port ', 'port number').env('PORT')) .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat)) - .addOption(new Option('--disable-server', 'disables the server').conflicts(['port'])); + .addOption(new Option('--disable-server', 'disables the server').conflicts(['port'])); // or `.conflicts('port')` for a single conflict program.parse(); diff --git a/lib/option.js b/lib/option.js index 42159c745..0cb6f12d9 100644 --- a/lib/option.js +++ b/lib/option.js @@ -68,14 +68,18 @@ class Option { } /** - * Set options names that are conflicts with this option. + * Set options name(s) that conflict with this option. * - * @param {string[]} names + * @param {string | string[]} names * @return {Option} */ conflicts(names) { - this.conflictsWith = names; + if (!Array.isArray(names) && typeof names !== 'string') { + throw new Error('conflicts() argument must be a string or an array of strings'); + } + + this.conflictsWith = this.conflictsWith.concat(names); return this; } diff --git a/tests/command.conflicts.test.js b/tests/command.conflicts.test.js index 4c14df3b5..48e4ae501 100644 --- a/tests/command.conflicts.test.js +++ b/tests/command.conflicts.test.js @@ -144,7 +144,7 @@ describe('command with conflicting options', () => { program .command('bar') .addOption(new commander.Option('--red')) - .addOption(new commander.Option('--dual').env('DUAL').conflicts('red')) + .addOption(new commander.Option('--dual').env('DUAL').conflicts(['red'])) .addOption(new commander.Option('--no-dual').env('NO_DUAL')); expect(() => { @@ -158,7 +158,7 @@ describe('command with conflicting options', () => { program .command('bar') .addOption(new commander.Option('--red')) - .addOption(new commander.Option('--dual').env('DUAL').conflicts('red')) + .addOption(new commander.Option('--dual').env('DUAL').conflicts(['red'])) .addOption(new commander.Option('--no-dual').env('NO_DUAL')); expect(() => { @@ -172,7 +172,7 @@ describe('command with conflicting options', () => { program .command('bar') .addOption(new commander.Option('--red')) - .addOption(new commander.Option('--dual').env('DUAL').conflicts('red')) + .addOption(new commander.Option('--dual').env('DUAL').conflicts(['red'])) .addOption(new commander.Option('--no-dual').env('NO_DUAL')); process.env.DUAL = 'true'; @@ -187,7 +187,7 @@ describe('command with conflicting options', () => { program .command('bar') .addOption(new commander.Option('--red')) - .addOption(new commander.Option('--dual').env('DUAL').conflicts('red')) + .addOption(new commander.Option('--dual').env('DUAL').conflicts(['red'])) .addOption(new commander.Option('--no-dual').env('NO_DUAL')); process.env.NO_DUAL = 'true'; @@ -202,7 +202,7 @@ describe('command with conflicting options', () => { program .command('bar') .addOption(new commander.Option('--red')) - .addOption(new commander.Option('--dual2 ').conflicts('red')) + .addOption(new commander.Option('--dual2 ').conflicts(['red'])) .addOption(new commander.Option('--no-dual2').preset('BAD')); expect(() => { @@ -216,11 +216,41 @@ describe('command with conflicting options', () => { program .command('bar') .addOption(new commander.Option('--red')) - .addOption(new commander.Option('--dual2 ').conflicts('red')) + .addOption(new commander.Option('--dual2 ').conflicts(['red'])) .addOption(new commander.Option('--no-dual2').preset('BAD')); expect(() => { program.parse('node test.js bar --red --no-dual2'.split(' ')); }).toThrow("error: option '--no-dual2' cannot be used with option '--red'"); }); + + test('should not throw error on when conflicts is invoked with a single string that includes another option', () => { + const { program } = makeProgram(); + + const actionMock = jest.fn(); + + program + .command('bar') + .addOption(new commander.Option('--a')) + .addOption(new commander.Option('--b').conflicts('aa')) + .action(actionMock); + + program.parse('node test.js bar --a --b'.split(' ')); + + expect(actionMock).toHaveBeenCalledTimes(1); + expect(actionMock).toHaveBeenCalledWith({ a: true, b: true }, expect.any(Object)); + }); + + test('should throw error on when conflicts is invoked with a single string that equals another option', () => { + const { program } = makeProgram(); + + program + .command('bar') + .addOption(new commander.Option('--a')) + .addOption(new commander.Option('--b').conflicts('a')); + + expect(() => { + program.parse('node test.js bar --a --b'.split(' ')); + }).toThrow("error: option '--b' cannot be used with option '--a'"); + }); }); diff --git a/typings/index.d.ts b/typings/index.d.ts index 1f1689b07..96564ec21 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -119,9 +119,9 @@ export class Option { preset(arg: unknown): this; /** - * Set options names that conflict with this option. + * Set options name(s) that conflict with this option. */ - conflicts(names: string[]): this; + conflicts(names: string | string[]): this; /** * Set environment variable to check for option value. diff --git a/typings/index.test-d.ts b/typings/index.test-d.ts index 9da76b62b..b1dc94ccf 100644 --- a/typings/index.test-d.ts +++ b/typings/index.test-d.ts @@ -415,6 +415,7 @@ expectType(baseOption.choices(['a', 'b'])); expectType(baseOption.choices(['a', 'b'] as const)); // conflicts +expectType(baseOption.conflicts('a')); expectType(baseOption.conflicts(['a', 'b'])); // name From e72e1c9875b2242b94ad669b365691e38d6bdaa1 Mon Sep 17 00:00:00 2001 From: erezrokah Date: Mon, 21 Feb 2022 23:33:06 +0100 Subject: [PATCH 20/20] test: fix typo in tests names --- tests/command.conflicts.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/command.conflicts.test.js b/tests/command.conflicts.test.js index 48e4ae501..8e2ce9860 100644 --- a/tests/command.conflicts.test.js +++ b/tests/command.conflicts.test.js @@ -224,7 +224,7 @@ describe('command with conflicting options', () => { }).toThrow("error: option '--no-dual2' cannot be used with option '--red'"); }); - test('should not throw error on when conflicts is invoked with a single string that includes another option', () => { + test('should not throw error when conflicts is invoked with a single string that includes another option', () => { const { program } = makeProgram(); const actionMock = jest.fn(); @@ -241,7 +241,7 @@ describe('command with conflicting options', () => { expect(actionMock).toHaveBeenCalledWith({ a: true, b: true }, expect.any(Object)); }); - test('should throw error on when conflicts is invoked with a single string that equals another option', () => { + test('should throw error when conflicts is invoked with a single string that equals another option', () => { const { program } = makeProgram(); program