diff --git a/Readme.md b/Readme.md index 992b3bdc7..7743cad7b 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('--disable-server', 'disables the server').conflicts(['port'])); // or `.conflicts('port')` for a single conflict ``` ```bash @@ -409,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) + --disable-server disables the server -h, --help display help for command $ extra --drink huge @@ -416,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 62c78debe..4f7a82390 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('--disable-server', 'disables the server').conflicts(['port'])); // or `.conflicts('port')` for a single conflict 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 bf31be4a3..cd6614f0c 100644 --- a/lib/command.js +++ b/lib/command.js @@ -1217,6 +1217,7 @@ Expecting one of '${allowedValues.join("', '")}'`); outputHelpIfRequested(this, parsed.unknown); this._checkForMissingMandatoryOptions(); + this._checkForConflictingOptions(); // We do not always call this check to avoid masking a "better" error, like unknown command. const checkForUnknownOptions = () => { @@ -1309,6 +1310,36 @@ Expecting one of '${allowedValues.join("', '")}'`); } } + /** + * Display an error message if conflicting options are used together. + * + * @api private + */ + _checkForConflictingOptions() { + const definedNonDefaultOptions = this.options.filter( + (option) => { + const optionKey = option.attributeName(); + if (this.getOptionValue(optionKey) === undefined) { + return false; + } + return this.getOptionValueSource(optionKey) !== 'default'; + } + ); + + const optionsWithConflicting = definedNonDefaultOptions.filter( + (option) => option.conflictsWith.length > 0 + ); + + optionsWithConflicting.forEach((option) => { + const conflictingAndDefined = definedNonDefaultOptions.find((defined) => + option.conflictsWith.includes(defined.attributeName()) + ); + if (conflictingAndDefined) { + this._conflictingOption(option, conflictingAndDefined); + } + }); + } + /** * Parse options from `argv` removing known options, * and return argv split into operands and unknown arguments. @@ -1560,6 +1591,44 @@ Expecting one of '${allowedValues.join("', '")}'`); this.error(message, { code: 'commander.missingMandatoryOptionValue' }); } + /** + * `Option` conflicts with another option. + * + * @param {Option} option + * @param {Option} conflictingOption + * @api private + */ + _conflictingOption(option, conflictingOption) { + // 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 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 bestOption = findBestOptionFromValue(option); + const optionKey = bestOption.attributeName(); + const source = this.getOptionValueSource(optionKey); + if (source === 'env') { + return `environment variable '${bestOption.envVar}'`; + } + return `option '${bestOption.flags}'`; + }; + + const message = `error: ${getErrorMessage(option)} cannot be used with ${getErrorMessage(conflictingOption)}`; + this.error(message, { code: 'commander.conflictingOption' }); + } + /** * Unknown option `flag`. * diff --git a/lib/option.js b/lib/option.js index c68110e4a..0cb6f12d9 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.conflictsWith = []; } /** @@ -66,6 +67,22 @@ class Option { return this; } + /** + * Set options name(s) that conflict with this option. + * + * @param {string | string[]} names + * @return {Option} + */ + + conflicts(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; + } + /** * Set environment variable to check for option value. * Priority order of option values is default < env < cli diff --git a/tests/command.conflicts.test.js b/tests/command.conflicts.test.js new file mode 100644 index 000000000..8e2ce9860 --- /dev/null +++ b/tests/command.conflicts.test.js @@ -0,0 +1,256 @@ +const commander = require('../'); + +describe('command with conflicting options', () => { + function makeProgram() { + const actionMock = jest.fn(); + const program = new commander.Command(); + program + .exitOverride() + .configureOutput({ + writeErr: () => {} + }) + .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').conflicts([ + 'silent' + ]) + ) + .action(actionMock); + + return { program, actionMock }; + } + + beforeEach(() => { + delete process.env.SILENT; + delete process.env.JSON; + delete process.env.DUAL; + delete process.env.NO_DUAL; + }); + + 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 conflicting 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 conflicting 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 option '-s, --silent'"); + }); + + test('should report the env variable as the conflicting option source, when conflicting 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 conflicting 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 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).conflicts(['silent'])); + + program.parse('node test.js foo --silent'.split(' ')); + + 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'"); + }); + + 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'"); + }); + + 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'"); + }); + + test('should not throw error 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 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/tests/command.exitOverride.test.js b/tests/command.exitOverride.test.js index 09b7b4989..08614e180 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.conflictingOption', "error: option '--debug' cannot be used with option '--silent'"); + }); + test('when call error() then throw CommanderError', () => { const program = new commander.Command(); program 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); + }); }); diff --git a/typings/index.d.ts b/typings/index.d.ts index cb7bf608b..96564ec21 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -118,6 +118,11 @@ export class Option { */ preset(arg: unknown): this; + /** + * Set options name(s) that conflict with this option. + */ + conflicts(names: string | string[]): this; + /** * Set environment variable to check for option value. * Priority order of option values is default < env < cli diff --git a/typings/index.test-d.ts b/typings/index.test-d.ts index a5536e542..b1dc94ccf 100644 --- a/typings/index.test-d.ts +++ b/typings/index.test-d.ts @@ -414,6 +414,10 @@ expectType(baseOption.hideHelp(false)); expectType(baseOption.choices(['a', 'b'])); expectType(baseOption.choices(['a', 'b'] as const)); +// conflicts +expectType(baseOption.conflicts('a')); +expectType(baseOption.conflicts(['a', 'b'])); + // name expectType(baseOption.name());