From 0504945ee161e464365645a12ae96d7644162714 Mon Sep 17 00:00:00 2001 From: John Gee Date: Sun, 11 Apr 2021 15:26:46 +1200 Subject: [PATCH] Check command-arguments even without action handler (#1502) * Check command-arguments even without action handler * Separate checking number of arguments and collecting variadic --- index.js | 38 ++++++---- tests/command.allowExcessArguments.test.js | 80 +++++++++------------- tests/command.exitOverride.test.js | 17 +++++ 3 files changed, 74 insertions(+), 61 deletions(-) diff --git a/index.js b/index.js index 9c56eab07..6178dde99 100644 --- a/index.js +++ b/index.js @@ -1559,28 +1559,38 @@ class Command extends EventEmitter { this.unknownOption(parsed.unknown[0]); } }; - - const commandEvent = `command:${this.name()}`; - if (this._actionHandler) { - checkForUnknownOptions(); - // Check expected arguments and collect variadic together. - const args = this.args.slice(); + const checkNumberOfArguments = () => { + // too few this._args.forEach((arg, i) => { - if (arg.required && args[i] == null) { + if (arg.required && this.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); + // too many + if (this._args.length > 0 && this._args[this._args.length - 1].variadic) { + return; + } + if (this.args.length > this._args.length) { + this._excessArguments(this.args); } + }; - this._actionHandler(args); + const commandEvent = `command:${this.name()}`; + if (this._actionHandler) { + checkForUnknownOptions(); + checkNumberOfArguments(); + // Collect trailing args into variadic. + let actionArgs = this.args; + const declaredArgCount = this._args.length; + if (declaredArgCount > 0 && this._args[declaredArgCount - 1].variadic) { + actionArgs = this.args.slice(0, declaredArgCount - 1); + actionArgs[declaredArgCount - 1] = this.args.slice(declaredArgCount - 1); + } + this._actionHandler(actionArgs); if (this.parent) this.parent.emit(commandEvent, operands, unknown); // legacy } else if (this.parent && this.parent.listenerCount(commandEvent)) { checkForUnknownOptions(); + checkNumberOfArguments(); this.parent.emit(commandEvent, operands, unknown); // legacy } else if (operands.length) { if (this._findCommand('*')) { // legacy default command @@ -1592,12 +1602,14 @@ class Command extends EventEmitter { this.unknownCommand(); } else { checkForUnknownOptions(); + checkNumberOfArguments(); } } else if (this.commands.length) { // This command has subcommands and nothing hooked up at this level, so display help. this.help({ error: true }); } else { checkForUnknownOptions(); + checkNumberOfArguments(); // fall through for caller to handle after calling .parse() } } diff --git a/tests/command.allowExcessArguments.test.js b/tests/command.allowExcessArguments.test.js index 99fbaf212..169772009 100644 --- a/tests/command.allowExcessArguments.test.js +++ b/tests/command.allowExcessArguments.test.js @@ -2,27 +2,21 @@ 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(); - }); +describe.each([true, false])('allowExcessArguments when action handler: %s', (hasActionHandler) => { + function configureCommand(cmd) { + cmd + .exitOverride() + .configureOutput({ + writeErr: () => {} + }); + if (hasActionHandler) { + cmd.action(() => {}); + } + } test('when specify excess program argument then no error by default', () => { const program = new commander.Command(); - program - .exitOverride() - .action(() => {}); + configureCommand(program); expect(() => { program.parse(['excess'], { from: 'user' }); @@ -31,10 +25,9 @@ describe('allowUnknownOption', () => { test('when specify excess program argument and allowExcessArguments(false) then error', () => { const program = new commander.Command(); + configureCommand(program); program - .exitOverride() - .allowExcessArguments(false) - .action(() => {}); + .allowExcessArguments(false); expect(() => { program.parse(['excess'], { from: 'user' }); @@ -43,10 +36,9 @@ describe('allowUnknownOption', () => { test('when specify excess program argument and allowExcessArguments() then no error', () => { const program = new commander.Command(); + configureCommand(program); program - .exitOverride() - .allowExcessArguments() - .action(() => {}); + .allowExcessArguments(); expect(() => { program.parse(['excess'], { from: 'user' }); @@ -55,10 +47,9 @@ describe('allowUnknownOption', () => { test('when specify excess program argument and allowExcessArguments(true) then no error', () => { const program = new commander.Command(); + configureCommand(program); program - .exitOverride() - .allowExcessArguments(true) - .action(() => {}); + .allowExcessArguments(true); expect(() => { program.parse(['excess'], { from: 'user' }); @@ -67,10 +58,9 @@ describe('allowUnknownOption', () => { test('when specify excess command argument then no error (by default)', () => { const program = new commander.Command(); - program - .exitOverride() - .command('sub') - .action(() => { }); + const sub = program + .command('sub'); + configureCommand(sub); expect(() => { program.parse(['sub', 'excess'], { from: 'user' }); @@ -79,12 +69,10 @@ describe('allowUnknownOption', () => { test('when specify excess command argument and allowExcessArguments(false) then error', () => { const program = new commander.Command(); - program - .exitOverride() + const sub = program .command('sub') - .allowUnknownOption() - .allowExcessArguments(false) - .action(() => { }); + .allowExcessArguments(false); + configureCommand(sub); expect(() => { program.parse(['sub', 'excess'], { from: 'user' }); @@ -93,11 +81,10 @@ describe('allowUnknownOption', () => { test('when specify expected arg and allowExcessArguments(false) then no error', () => { const program = new commander.Command(); + configureCommand(program); program .argument('') - .exitOverride() - .allowExcessArguments(false) - .action(() => {}); + .allowExcessArguments(false); expect(() => { program.parse(['file'], { from: 'user' }); @@ -106,11 +93,10 @@ describe('allowUnknownOption', () => { test('when specify excess after and allowExcessArguments(false) then error', () => { const program = new commander.Command(); + configureCommand(program); program .argument('') - .exitOverride() - .allowExcessArguments(false) - .action(() => {}); + .allowExcessArguments(false); expect(() => { program.parse(['file', 'excess'], { from: 'user' }); @@ -119,11 +105,10 @@ describe('allowUnknownOption', () => { test('when specify excess after [arg] and allowExcessArguments(false) then error', () => { const program = new commander.Command(); + configureCommand(program); program .argument('[file]') - .exitOverride() - .allowExcessArguments(false) - .action(() => {}); + .allowExcessArguments(false); expect(() => { program.parse(['file', 'excess'], { from: 'user' }); @@ -132,11 +117,10 @@ describe('allowUnknownOption', () => { test('when specify args for [args...] and allowExcessArguments(false) then no error', () => { const program = new commander.Command(); + configureCommand(program); program .argument('[files...]') - .exitOverride() - .allowExcessArguments(false) - .action(() => {}); + .allowExcessArguments(false); expect(() => { program.parse(['file1', 'file2', 'file3'], { from: 'user' }); diff --git a/tests/command.exitOverride.test.js b/tests/command.exitOverride.test.js index a951baeef..6bf3b134b 100644 --- a/tests/command.exitOverride.test.js +++ b/tests/command.exitOverride.test.js @@ -120,6 +120,23 @@ describe('.exitOverride and error details', () => { expectCommanderError(caughtErr, 1, 'commander.missingArgument', "error: missing required argument 'arg-name'"); }); + test('when specify program without required argument and no action handler then throw CommanderError', () => { + const program = new commander.Command(); + program + .exitOverride() + .argument(''); + + let caughtErr; + try { + program.parse(['node', 'test']); + } catch (err) { + caughtErr = err; + } + + expect(stderrSpy).toHaveBeenCalled(); + 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