WIP: Reject excess arguments by default

index.js

@@ -527,6 +527,7 @@ class Command extends EventEmitter {
     this.options = [];
     this.parent = null;
     this._allowUnknownOption = false;
+    this._allowExcessArguments = false;
     this._args = [];
     this.rawArgs = null;
     this._scriptPath = null;
@@ -882,18 +883,20 @@ class Command extends EventEmitter {
 
   action(fn) {
     const listener = (args) => {
-      // The .action callback takes an extra parameter which is the command or options.
       const expectedArgsCount = this._args.length;
       const actionArgs = args.slice(0, expectedArgsCount);
+      if (args.length > expectedArgsCount && this._actionHandler) {
+        this.excessArguments(args.slice(expectedArgsCount));
+      }
+
+      // Add the options (which might be the command, with options as properties!)
       if (this._passCommandToAction) {
         actionArgs[expectedArgsCount] = this;
       } else {
         actionArgs[expectedArgsCount] = this.opts();
       }
-      // Add the extra arguments so available too.
-      if (args.length > expectedArgsCount) {
-        actionArgs.push(args.slice(expectedArgsCount));
-      }
+      // Add command
+      actionArgs.push(this);
 
       const actionResult = fn.apply(this, actionArgs);
       // Remember result in case it is async. Assume parseAsync getting called on root.
@@ -1161,6 +1164,17 @@ Read more on https://git.io/JJc0W`);
     return this;
   };
 
+  /**
+   * Allow excess arguments on the command line, beyond what has been declared.
+   *
+   * @param {Boolean} [allowExcess] - if `true` or omitted, no error will be thrown
+   * for unknown 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().
@@ -1741,6 +1755,20 @@ Read more on https://git.io/JJc0W`);
     this._displayError(1, 'commander.unknownOption', message);
   };
 
+  /**
+   * Excess arguments `arg`.
+   *
+   * @param {string[]} args
+   * @api private
+   */
+
+  excessArguments(args) {
+    if (this._allowExcessArguments || args.length === 0) return;
+    if (this.name() === '*') return; // support legacy use, do not enforce arguments
+    const message = `error: excess argument${args.length > 1 ? 's' : ''} '${args.join(' ')}'`;
+    this._displayError(1, 'commander.excessArguments', message);
+  };
+
   /**
    * Unknown command.
    *

tests/args.variadic.test.js

@@ -12,7 +12,7 @@ describe('variadic argument', () => {
 
     program.parse(['node', 'test', 'id']);
 
-    expect(actionMock).toHaveBeenCalledWith('id', [], program);
+    expect(actionMock).toHaveBeenCalledWith('id', [], program, program);
   });
 
   test('when extra arguments specified for program then variadic arg is array of values', () => {
@@ -25,7 +25,7 @@ describe('variadic argument', () => {
 
     program.parse(['node', 'test', 'id', ...extraArguments]);
 
-    expect(actionMock).toHaveBeenCalledWith('id', extraArguments, program);
+    expect(actionMock).toHaveBeenCalledWith('id', extraArguments, program, program);
   });
 
   test('when no extra arguments specified for command then variadic arg is empty array', () => {
@@ -37,7 +37,7 @@ describe('variadic argument', () => {
 
     program.parse(['node', 'test', 'sub']);
 
-    expect(actionMock).toHaveBeenCalledWith([], cmd);
+    expect(actionMock).toHaveBeenCalledWith([], cmd, cmd);
   });
 
   test('when extra arguments specified for command then variadic arg is array of values', () => {
@@ -50,7 +50,7 @@ describe('variadic argument', () => {
 
     program.parse(['node', 'test', 'sub', ...extraArguments]);
 
-    expect(actionMock).toHaveBeenCalledWith(extraArguments, cmd);
+    expect(actionMock).toHaveBeenCalledWith(extraArguments, cmd, cmd);
   });
 
   test('when program variadic argument not last then error', () => {

tests/command.action.test.js

@@ -9,7 +9,7 @@ test('when .action called then command passed to action', () => {
     .command('info')
     .action(actionMock);
   program.parse(['node', 'test', 'info']);
-  expect(actionMock).toHaveBeenCalledWith(cmd);
+  expect(actionMock).toHaveBeenCalledWith(cmd, cmd);
 });
 
 test('when .action called then program.args only contains args', () => {
@@ -23,16 +23,31 @@ test('when .action called then program.args only contains args', () => {
   expect(program.args).toEqual(['info', 'my-file']);
 });
 
-test('when .action called with extra arguments then extras also passed to action', () => {
-  // This is a new and undocumented behaviour for now.
-  // Might make this an error by default in future.
+test('when .action called with excess arguments then error', () => {
   const actionMock = jest.fn();
   const program = new commander.Command();
-  const cmd = program
+  program.exitOverride();
+  program
     .command('info <file>')
+    .configureOutput({ writeErr: () => {} })
     .action(actionMock);
-  program.parse(['node', 'test', 'info', 'my-file', 'a']);
-  expect(actionMock).toHaveBeenCalledWith('my-file', cmd, ['a']);
+  expect(() => {
+    program.parse(['node', 'test', 'info', 'my-file', 'a']);
+  }).toThrow();
+});
+
+test('when .action called with allowed excess arguments then no error', () => {
+  const actionMock = jest.fn();
+  const program = new commander.Command();
+  program.exitOverride();
+  program
+    .command('info <file>')
+    .allowExcessArguments()
+    .action(actionMock);
+  expect(() => {
+    program.parse(['node', 'test', 'info', 'my-file', 'a']);
+  }).not.toThrow();
+  expect(actionMock).toHaveBeenCalled();
 });
 
 test('when .action on program with required argument and argument supplied then action called', () => {
@@ -42,7 +57,7 @@ test('when .action on program with required argument and argument supplied then
     .arguments('<file>')
     .action(actionMock);
   program.parse(['node', 'test', 'my-file']);
-  expect(actionMock).toHaveBeenCalledWith('my-file', program);
+  expect(actionMock).toHaveBeenCalledWith('my-file', program, program);
 });
 
 test('when .action on program with required argument and argument not supplied then action not called', () => {
@@ -66,7 +81,7 @@ test('when .action on program and no arguments then action called', () => {
   program
     .action(actionMock);
   program.parse(['node', 'test']);
-  expect(actionMock).toHaveBeenCalledWith(program);
+  expect(actionMock).toHaveBeenCalledWith(program, program);
 });
 
 test('when .action on program with optional argument supplied then action called', () => {
@@ -76,7 +91,7 @@ test('when .action on program with optional argument supplied then action called
     .arguments('[file]')
     .action(actionMock);
   program.parse(['node', 'test', 'my-file']);
-  expect(actionMock).toHaveBeenCalledWith('my-file', program);
+  expect(actionMock).toHaveBeenCalledWith('my-file', program, program);
 });
 
 test('when .action on program without optional argument supplied then action called', () => {
@@ -86,7 +101,7 @@ test('when .action on program without optional argument supplied then action cal
     .arguments('[file]')
     .action(actionMock);
   program.parse(['node', 'test']);
-  expect(actionMock).toHaveBeenCalledWith(undefined, program);
+  expect(actionMock).toHaveBeenCalledWith(undefined, program, program);
 });
 
 test('when .action on program with optional argument and subcommand and program argument then program action called', () => {
@@ -100,7 +115,7 @@ test('when .action on program with optional argument and subcommand and program
 
   program.parse(['node', 'test', 'a']);
 
-  expect(actionMock).toHaveBeenCalledWith('a', program);
+  expect(actionMock).toHaveBeenCalledWith('a', program, program);
 });
 
 // Changes made in #1062 to allow this case
@@ -115,7 +130,7 @@ test('when .action on program with optional argument and subcommand and no progr
 
   program.parse(['node', 'test']);
 
-  expect(actionMock).toHaveBeenCalledWith(undefined, program);
+  expect(actionMock).toHaveBeenCalledWith(undefined, program, program);
 });
 
 test('when action is async then can await parseAsync', async() => {

tests/command.allowUnknownOption.test.js

@@ -84,6 +84,7 @@ describe('allowUnknownOption', () => {
       .exitOverride()
       .command('sub')
       .allowUnknownOption()
+      .arguments('<args...>')
       .option('-p, --pepper', 'add pepper')
       .action(() => { });
 

tests/command.default.test.js

@@ -34,6 +34,7 @@ describe('default action command', () => {
     program
       .command('default', { isDefault: true })
       .allowUnknownOption()
+      .allowExcessArguments()
       .action(actionMock);
     return { program, actionMock };
   }
@@ -62,6 +63,7 @@ describe('default added command', () => {
     const actionMock = jest.fn();
     const defaultCmd = new commander.Command('default')
       .allowUnknownOption()
+      .allowExcessArguments()
       .action(actionMock);
 
     const program = new commander.Command();

tests/command.exitOverride.test.js

@@ -47,6 +47,24 @@ describe('.exitOverride and error details', () => {
     expectCommanderError(caughtErr, 1, 'commander.unknownOption', "error: unknown option '-m'");
   });
 
+  test('when specify excess program arguments then throw CommanderError', () => {
+    const program = new commander.Command();
+    program
+      .arguments('<file>')
+      .exitOverride()
+      .action(() => {});
+
+    let caughtErr;
+    try {
+      program.parse(['node', 'test', 'expected', 'excess']);
+    } catch (err) {
+      caughtErr = err;
+    }
+
+    expect(stderrSpy).toHaveBeenCalled();
+    expectCommanderError(caughtErr, 1, 'commander.excessArguments', "error: excess argument 'excess'");
+  });
+
   test('when specify unknown command then throw CommanderError', () => {
     const program = new commander.Command();
     program

tests/command.unknownCommand.test.js

@@ -24,7 +24,7 @@ describe('unknownOption', () => {
     }).not.toThrow();
   });
 
-  test('when unknown command but action handler then no error', () => {
+  test('when unknown command and program action handler then error due to excess argument', () => {
     const program = new commander.Command();
     program
       .exitOverride()
@@ -33,7 +33,7 @@ describe('unknownOption', () => {
       .action(() => { });
     expect(() => {
       program.parse('node test.js unknown'.split(' '));
-    }).not.toThrow();
+    }).toThrow();
   });
 
   test('when unknown command but listener then no error', () => {

tests/commander.configureCommand.test.js

@@ -19,7 +19,7 @@ test('when default then command passed to action', () => {
     .arguments('<value>')
     .action(callback);
   program.parse(['node', 'test', 'value']);
-  expect(callback).toHaveBeenCalledWith('value', program);
+  expect(callback).toHaveBeenCalledWith('value', program, program);
 });
 
 // storeOptionsAsProperties
@@ -61,7 +61,7 @@ test('when passCommandToAction() then command passed to action', () => {
     .arguments('<value>')
     .action(callback);
   program.parse(['node', 'test', 'value']);
-  expect(callback).toHaveBeenCalledWith('value', program);
+  expect(callback).toHaveBeenCalledWith('value', program, program);
 });
 
 test('when passCommandToAction(true) then command passed to action', () => {
@@ -72,7 +72,7 @@ test('when passCommandToAction(true) then command passed to action', () => {
     .arguments('<value>')
     .action(callback);
   program.parse(['node', 'test', 'value']);
-  expect(callback).toHaveBeenCalledWith('value', program);
+  expect(callback).toHaveBeenCalledWith('value', program, program);
 });
 
 test('when passCommandToAction(false) then options passed to action', () => {
@@ -83,5 +83,5 @@ test('when passCommandToAction(false) then options passed to action', () => {
     .arguments('<value>')
     .action(callback);
   program.parse(['node', 'test', 'value']);
-  expect(callback).toHaveBeenCalledWith('value', program.opts());
+  expect(callback).toHaveBeenCalledWith('value', program.opts(), program);
 });