From 08688d1696c328c3dc9299c9b539162561e2ceea Mon Sep 17 00:00:00 2001 From: John Gee Date: Wed, 2 Dec 2020 14:24:04 +1300 Subject: [PATCH 1/7] Fix test file name --- ...elp.visibleArgumnets.test.js => help.visibleArguments.test.js} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename tests/{help.visibleArgumnets.test.js => help.visibleArguments.test.js} (100%) diff --git a/tests/help.visibleArgumnets.test.js b/tests/help.visibleArguments.test.js similarity index 100% rename from tests/help.visibleArgumnets.test.js rename to tests/help.visibleArguments.test.js From 5ea77748049b9293462b0d7158125a23264facc9 Mon Sep 17 00:00:00 2001 From: John Gee Date: Wed, 2 Dec 2020 14:25:02 +1300 Subject: [PATCH 2/7] Add initial support for allowExcessArguments --- index.js | 34 ++++++++++++++++++++++++++++++ tests/command.chain.test.js | 6 ++++++ tests/command.exitOverride.test.js | 19 +++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/index.js b/index.js index ed23283d6..d89d2b8c3 100644 --- a/index.js +++ b/index.js @@ -535,6 +535,7 @@ class Command extends EventEmitter { this.options = []; this.parent = null; this._allowUnknownOption = false; + this._allowExcessArguments = true; this._args = []; this.rawArgs = null; this._scriptPath = null; @@ -1170,6 +1171,17 @@ Read more on https://git.io/JJc0W`); return this; }; + /** + * Allow excess arguments on the command line. + * + * @param {Boolean} [allowExcess] - if `true` or omitted, no error will be thrown + * for excess arguments. + */ + allowExcessArguments(allowExcess) { + this._allowExcessArguments = (allowExcess === undefined) || !!allowExcess; + return this; + }; + /** * Whether to store option values as properties on command object, * or store separately (specify false). In both cases the option values can be accessed using .opts(). @@ -1492,14 +1504,19 @@ Read more on https://git.io/JJc0W`); const commandEvent = `command:${this.name()}`; if (this._actionHandler) { + // Check expected arguments and collect variadic together. const args = this.args.slice(); this._args.forEach((arg, i) => { if (arg.required && args[i] == null) { this.missingArgument(arg.name); } else if (arg.variadic) { args[i] = args.splice(i); + args.length = Math.min(i + 1, args.length); } }); + if (args.length > this._args.length) { + this._excessArguments(args); // may be allowed + } this._actionHandler(args); if (this.parent) this.parent.emit(commandEvent, operands, unknown); // legacy @@ -1753,6 +1770,23 @@ Read more on https://git.io/JJc0W`); this._displayError(1, 'commander.unknownOption', message); }; + /** + * Excess arguments, more than expected. + * + * @param {string[]} receivedArgs + * @api private + */ + + _excessArguments(receivedArgs) { + if (this._allowExcessArguments) return; + if (this._name === '*') return; // Legacy default handler + + const expected = this._args.length; + const s = expected === 1 ? '' : 's'; + const message = `error: too many arguments for '${this.name()}'. Expected ${expected} argument${s} but got ${receivedArgs.length}.`; + this._displayError(1, 'commander.excessArguments', message); + }; + /** * Unknown command. * diff --git a/tests/command.chain.test.js b/tests/command.chain.test.js index 9918dae0c..7bf538740 100644 --- a/tests/command.chain.test.js +++ b/tests/command.chain.test.js @@ -70,6 +70,12 @@ describe('Command methods that should return this for chaining', () => { expect(result).toBe(program); }); + test('when call .allowExcessArguments() then returns this', () => { + const program = new Command(); + const result = program.allowExcessArguments(); + expect(result).toBe(program); + }); + test('when call .storeOptionsAsProperties() then returns this', () => { const program = new Command(); const result = program.storeOptionsAsProperties(); diff --git a/tests/command.exitOverride.test.js b/tests/command.exitOverride.test.js index 40ab1783f..b81b0a2e9 100644 --- a/tests/command.exitOverride.test.js +++ b/tests/command.exitOverride.test.js @@ -120,6 +120,25 @@ describe('.exitOverride and error details', () => { expectCommanderError(caughtErr, 1, 'commander.missingArgument', "error: missing required argument 'arg-name'"); }); + test('when specify command with excess argument then throw CommanderError', () => { + const program = new commander.Command(); + program + .exitOverride() + .command('speak') + .allowExcessArguments(false) + .action(() => { }); + + let caughtErr; + try { + program.parse(['node', 'test', 'speak', 'excess']); + } catch (err) { + caughtErr = err; + } + + expect(stderrSpy).toHaveBeenCalled(); + expectCommanderError(caughtErr, 1, 'commander.excessArguments', "error: too many arguments for 'speak'. Expected 0 arguments but got 1."); + }); + test('when specify --help then throw CommanderError', () => { const writeSpy = jest.spyOn(process.stdout, 'write').mockImplementation(() => { }); const program = new commander.Command(); From 636767fcd3c19d3e306f765e7435b562ef751d9b Mon Sep 17 00:00:00 2001 From: John Gee Date: Wed, 2 Dec 2020 14:30:08 +1300 Subject: [PATCH 3/7] Add TypeScript --- typings/commander-tests.ts | 4 ++++ typings/index.d.ts | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/typings/commander-tests.ts b/typings/commander-tests.ts index fed35998c..fb5b9bb7a 100644 --- a/typings/commander-tests.ts +++ b/typings/commander-tests.ts @@ -159,6 +159,10 @@ const combineFlagAndOptionalValueThis2: commander.Command = program.combineFlagA const allowUnknownOptionThis1: commander.Command = program.allowUnknownOption(); const allowUnknownOptionThis2: commander.Command = program.allowUnknownOption(false); +// allowExcessArguments +const allowExcessArgumentsThis1: commander.Command = program.allowExcessArguments(); +const allowExcessArgumentsThis2: commander.Command = program.allowExcessArguments(false); + // parse const parseThis1: commander.Command = program.parse(); const parseThis2: commander.Command = program.parse(process.argv); diff --git a/typings/index.d.ts b/typings/index.d.ts index 1baa20922..ba6a5f860 100644 --- a/typings/index.d.ts +++ b/typings/index.d.ts @@ -403,6 +403,13 @@ declare namespace commander { */ allowUnknownOption(allowUnknown?: boolean): this; + /** + * Allow excess arguments on the command line. + * + * @returns `this` command for chaining + */ + allowExcessArguments(allowExcess?: boolean): this; + /** * Parse `argv`, setting options and invoking commands when defined. * From 7ae352da839e74dc4333f261b7a7665f84b76960 Mon Sep 17 00:00:00 2001 From: John Gee Date: Wed, 2 Dec 2020 14:50:40 +1300 Subject: [PATCH 4/7] Add tests --- tests/command.allowExcessArguments.test.js | 145 +++++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 tests/command.allowExcessArguments.test.js diff --git a/tests/command.allowExcessArguments.test.js b/tests/command.allowExcessArguments.test.js new file mode 100644 index 000000000..6ce203fc9 --- /dev/null +++ b/tests/command.allowExcessArguments.test.js @@ -0,0 +1,145 @@ +const commander = require('../'); + +// Not testing output, just testing whether an error is detected. + +describe('allowUnknownOption', () => { + // Optional. Use internal knowledge to suppress output to keep test output clean. + let writeErrorSpy; + + beforeAll(() => { + writeErrorSpy = jest.spyOn(process.stderr, 'write').mockImplementation(() => { }); + }); + + afterEach(() => { + writeErrorSpy.mockClear(); + }); + + afterAll(() => { + writeErrorSpy.mockRestore(); + }); + + test('when specify excess program argument then ignored by default', () => { + const program = new commander.Command(); + program + .exitOverride() + .action(() => {}); + + expect(() => { + program.parse(['excess'], { from: 'user' }); + }).not.toThrow(); + }); + + test('when specify excess program argument and allowExcessArguments(false) then error', () => { + const program = new commander.Command(); + program + .exitOverride() + .allowExcessArguments(false) + .action(() => {}); + + expect(() => { + program.parse(['excess'], { from: 'user' }); + }).toThrow(); + }); + + test('when specify excess program argument and allowExcessArguments() then no error', () => { + const program = new commander.Command(); + program + .exitOverride() + .allowExcessArguments() + .action(() => {}); + + expect(() => { + program.parse(['excess'], { from: 'user' }); + }).not.toThrow(); + }); + + test('when specify excess program argument and allowExcessArguments(true) then no error', () => { + const program = new commander.Command(); + program + .exitOverride() + .allowExcessArguments(true) + .action(() => {}); + + expect(() => { + program.parse(['excess'], { from: 'user' }); + }).not.toThrow(); + }); + + test('when specify excess command argument then no error (by default)', () => { + const program = new commander.Command(); + program + .exitOverride() + .command('sub') + .action(() => { }); + + expect(() => { + program.parse(['sub', 'excess'], { from: 'user' }); + }).not.toThrow(); + }); + + test('when specify excess command argument and allowExcessArguments(false) then error', () => { + const program = new commander.Command(); + program + .exitOverride() + .command('sub') + .allowUnknownOption() + .allowExcessArguments(false) + .action(() => { }); + + expect(() => { + program.parse(['sub', 'excess'], { from: 'user' }); + }).toThrow(); + }); + + test('when specify expected arg and allowExcessArguments(false) then no error', () => { + const program = new commander.Command(); + program + .arguments('') + .exitOverride() + .allowExcessArguments(false) + .action(() => {}); + + expect(() => { + program.parse(['file'], { from: 'user' }); + }).not.toThrow(); + }); + + test('when specify excess after and allowExcessArguments(false) then error', () => { + const program = new commander.Command(); + program + .arguments('') + .exitOverride() + .allowExcessArguments(false) + .action(() => {}); + + expect(() => { + program.parse(['file', 'excess'], { from: 'user' }); + }).toThrow(); + }); + + test('when specify excess after [arg] and allowExcessArguments(false) then error', () => { + const program = new commander.Command(); + program + .arguments('[file]') + .exitOverride() + .allowExcessArguments(false) + .action(() => {}); + + expect(() => { + program.parse(['file', 'excess'], { from: 'user' }); + }).toThrow(); + }); + + test('when specify args for [args...] and allowExcessArguments(false) then no error', () => { + const program = new commander.Command(); + program + .arguments('[files...]') + .exitOverride() + .allowExcessArguments(false) + .action(() => {}); + + expect(() => { + program.parse(['file1', 'file2', 'file3'], { from: 'user' }); + }).not.toThrow(); + }); +}); From 7cf8aa8faf6e5456d40556f54a9ea53dd5334258 Mon Sep 17 00:00:00 2001 From: John Gee Date: Wed, 2 Dec 2020 15:06:38 +1300 Subject: [PATCH 5/7] Refine error for program --- index.js | 5 +++-- tests/command.exitOverride.test.js | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index d89d2b8c3..8c209e51d 100644 --- a/index.js +++ b/index.js @@ -1782,8 +1782,9 @@ Read more on https://git.io/JJc0W`); if (this._name === '*') return; // Legacy default handler const expected = this._args.length; - const s = expected === 1 ? '' : 's'; - const message = `error: too many arguments for '${this.name()}'. Expected ${expected} argument${s} but got ${receivedArgs.length}.`; + const s = (expected === 1) ? '' : 's'; + const forSubcommand = this.parent ? ` for '${this.name()}'` : ''; + const message = `error: too many arguments${forSubcommand}. Expected ${expected} argument${s} but got ${receivedArgs.length}.`; this._displayError(1, 'commander.excessArguments', message); }; diff --git a/tests/command.exitOverride.test.js b/tests/command.exitOverride.test.js index b81b0a2e9..27b7c45c5 100644 --- a/tests/command.exitOverride.test.js +++ b/tests/command.exitOverride.test.js @@ -120,6 +120,24 @@ describe('.exitOverride and error details', () => { expectCommanderError(caughtErr, 1, 'commander.missingArgument', "error: missing required argument 'arg-name'"); }); + test('when specify excess argument then throw CommanderError', () => { + const program = new commander.Command(); + program + .exitOverride() + .allowExcessArguments(false) + .action(() => { }); + + let caughtErr; + try { + program.parse(['node', 'test', 'excess']); + } catch (err) { + caughtErr = err; + } + + expect(stderrSpy).toHaveBeenCalled(); + expectCommanderError(caughtErr, 1, 'commander.excessArguments', 'error: too many arguments. Expected 0 arguments but got 1.'); + }); + test('when specify command with excess argument then throw CommanderError', () => { const program = new commander.Command(); program From 6f0c80bee8789ed764a64b9ae5a278fa7db988c9 Mon Sep 17 00:00:00 2001 From: John Gee Date: Wed, 2 Dec 2020 18:55:34 +1300 Subject: [PATCH 6/7] Remove exception for legacy asterisk command, can disable if needed --- index.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/index.js b/index.js index 8c209e51d..0ded86f77 100644 --- a/index.js +++ b/index.js @@ -1515,7 +1515,7 @@ Read more on https://git.io/JJc0W`); } }); if (args.length > this._args.length) { - this._excessArguments(args); // may be allowed + this._excessArguments(args); } this._actionHandler(args); @@ -1779,7 +1779,6 @@ Read more on https://git.io/JJc0W`); _excessArguments(receivedArgs) { if (this._allowExcessArguments) return; - if (this._name === '*') return; // Legacy default handler const expected = this._args.length; const s = (expected === 1) ? '' : 's'; From a42d5c93e2a1b65f01d15a89cd81de14bfbcc763 Mon Sep 17 00:00:00 2001 From: John Gee Date: Thu, 3 Dec 2020 19:13:09 +1300 Subject: [PATCH 7/7] Use defaulr parameters and simplify code --- index.js | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/index.js b/index.js index 0ded86f77..c3db1d557 100644 --- a/index.js +++ b/index.js @@ -404,24 +404,24 @@ class Option { /** * Whether the option is mandatory and must have a value after parsing. * - * @param {boolean} [mandatory] + * @param {boolean} [mandatory=true] * @return {Option} */ - makeOptionMandatory(mandatory) { - this.mandatory = (mandatory === undefined) || !!mandatory; + makeOptionMandatory(mandatory = true) { + this.mandatory = !!mandatory; return this; }; /** * Hide option in help. * - * @param {boolean} [hide] + * @param {boolean} [hide=true] * @return {Option} */ - hideHelp(hide) { - this.hidden = (hide === undefined) || !!hide; + hideHelp(hide = true) { + this.hidden = !!hide; return this; }; @@ -1153,32 +1153,32 @@ Read more on https://git.io/JJc0W`); * .combineFlagAndOptionalValue(true) // `-f80` is treated like `--flag=80`, this is the default behaviour * .combineFlagAndOptionalValue(false) // `-fb` is treated like `-f -b` * - * @param {Boolean} [combine] - if `true` or omitted, an optional value can be specified directly after the flag. + * @param {Boolean} [combine=true] - if `true` or omitted, an optional value can be specified directly after the flag. */ - combineFlagAndOptionalValue(combine) { - this._combineFlagAndOptionalValue = (combine === undefined) || !!combine; + combineFlagAndOptionalValue(combine = true) { + this._combineFlagAndOptionalValue = !!combine; return this; }; /** * Allow unknown options on the command line. * - * @param {Boolean} [allowUnknown] - if `true` or omitted, no error will be thrown + * @param {Boolean} [allowUnknown=true] - if `true` or omitted, no error will be thrown * for unknown options. */ - allowUnknownOption(allowUnknown) { - this._allowUnknownOption = (allowUnknown === undefined) || !!allowUnknown; + allowUnknownOption(allowUnknown = true) { + this._allowUnknownOption = !!allowUnknown; return this; }; /** * Allow excess arguments on the command line. * - * @param {Boolean} [allowExcess] - if `true` or omitted, no error will be thrown + * @param {Boolean} [allowExcess=true] - if `true` or omitted, no error will be thrown * for excess arguments. */ - allowExcessArguments(allowExcess) { - this._allowExcessArguments = (allowExcess === undefined) || !!allowExcess; + allowExcessArguments(allowExcess = true) { + this._allowExcessArguments = !!allowExcess; return this; }; @@ -1186,13 +1186,13 @@ Read more on https://git.io/JJc0W`); * Whether to store option values as properties on command object, * or store separately (specify false). In both cases the option values can be accessed using .opts(). * - * @param {boolean} storeAsProperties + * @param {boolean} [storeAsProperties=true] * @return {Command} `this` command for chaining */ - storeOptionsAsProperties(storeAsProperties) { + storeOptionsAsProperties(storeAsProperties = true) { this._storeOptionsAsPropertiesCalled = true; - this._storeOptionsAsProperties = (storeAsProperties === undefined) || !!storeAsProperties; + this._storeOptionsAsProperties = !!storeAsProperties; if (this.options.length) { throw new Error('call .storeOptionsAsProperties() before adding options'); } @@ -1203,12 +1203,12 @@ Read more on https://git.io/JJc0W`); * Whether to pass command to action handler, * or just the options (specify false). * - * @param {boolean} passCommand + * @param {boolean} [passCommand=true] * @return {Command} `this` command for chaining */ - passCommandToAction(passCommand) { - this._passCommandToAction = (passCommand === undefined) || !!passCommand; + passCommandToAction(passCommand = true) { + this._passCommandToAction = !!passCommand; return this; };