Disallow excess arguments (#2223)

* Change default allowExcessArguments to false and fix tests
* Add migration tips for allowExcessArguments change

Author: shadowspawn

CHANGELOG.md

@@ -8,6 +8,54 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 <!-- markdownlint-disable MD024 -->
 <!-- markdownlint-disable MD004 -->
 
+## [13.x] (date goes here)
+
+### Changed
+
+- *Breaking*: excess command-arguments cause an error by default
+
+### Migration Tips
+
+**Excess command-arguments**
+
+It is now an error to pass more command-arguments than are expected.
+
+Old code:
+
+```js
+program.option('-p, --port <number>', 'port number');
+program.action((options) => {
+  console.log(program.args);
+});
+```
+
+```console
+$ node example.js a b c
+error: too many arguments. Expected 0 arguments but got 3.
+```
+
+You can declare the expected arguments. The help will then be more accurate too. Note that declaring
+new arguments will change what is passed to the action handler.
+
+```js
+program.option('-p, --port <number>', 'port number');
+program.argument('[args...]', 'remote command and arguments'); // expecting zero or more arguments
+program.action((args, options) => {
+  console.log(args);
+});
+```
+
+Or you could suppress the error without changing the rest of the code:
+
+```js
+program.option('-p, --port', 'port number');
+program.allowExcessArguments();
+program.action((options) => {
+  console.log(program.args);
+});
+```
+
+
 ## [12.1.0] (2024-05-18)
 
 ### Added

lib/command.js

@@ -25,7 +25,7 @@ class Command extends EventEmitter {
     this.options = [];
     this.parent = null;
     this._allowUnknownOption = false;
-    this._allowExcessArguments = true;
+    this._allowExcessArguments = false;
     /** @type {Argument[]} */
     this.registeredArguments = [];
     this._args = this.registeredArguments; // deprecated old name

tests/args.literal.test.js

@@ -9,7 +9,8 @@ test('when arguments includes -- then stop processing options', () => {
   const program = new commander.Command();
   program
     .option('-f, --foo', 'add some foo')
-    .option('-b, --bar', 'add some bar');
+    .option('-b, --bar', 'add some bar')
+    .argument('[args...]');
   program.parse(['node', 'test', '--foo', '--', '--bar', 'baz']);
   // More than one assert, ported from legacy test
   const opts = program.opts();
@@ -22,7 +23,8 @@ test('when arguments include -- then more literals are passed-through as args',
   const program = new commander.Command();
   program
     .option('-f, --foo', 'add some foo')
-    .option('-b, --bar', 'add some bar');
+    .option('-b, --bar', 'add some bar')
+    .argument('[args...]');
   program.parse(['node', 'test', '--', 'cmd', '--', '--arg']);
   expect(program.args).toEqual(['cmd', '--', '--arg']);
 });

tests/command.allowExcessArguments.test.js

@@ -14,13 +14,13 @@ describe.each([true, false])(
       }
     }
 
-    test('when specify excess program argument then no error by default', () => {
+    test('when specify excess program argument then error by default', () => {
       const program = new commander.Command();
       configureCommand(program);
 
       expect(() => {
         program.parse(['excess'], { from: 'user' });
-      }).not.toThrow();
+      }).toThrow();
     });
 
     test('when specify excess program argument and allowExcessArguments(false) then error', () => {
@@ -53,14 +53,14 @@ describe.each([true, false])(
       }).not.toThrow();
     });
 
-    test('when specify excess command argument then no error (by default)', () => {
+    test('when specify excess command argument then error (by default)', () => {
       const program = new commander.Command();
       const sub = program.command('sub');
       configureCommand(sub);
 
       expect(() => {
         program.parse(['sub', 'excess'], { from: 'user' });
-      }).not.toThrow();
+      }).toThrow();
     });
 
     test('when specify excess command argument and allowExcessArguments(false) then error', () => {

tests/command.allowUnknownOption.test.js

@@ -46,6 +46,7 @@ describe('allowUnknownOption', () => {
     program
       .exitOverride()
       .allowUnknownOption()
+      .argument('[args...]') // unknown option will be passed as an argument
       .option('-p, --pepper', 'add pepper');
 
     expect(() => {
@@ -58,6 +59,7 @@ describe('allowUnknownOption', () => {
     program
       .exitOverride()
       .allowUnknownOption(true)
+      .argument('[args...]') // unknown option will be passed as an argument
       .option('-p, --pepper', 'add pepper');
 
     expect(() => {

tests/command.copySettings.test.js

@@ -119,10 +119,10 @@ describe('copyInheritedSettings property tests', () => {
     const source = new commander.Command();
     const cmd = new commander.Command();
 
-    expect(cmd._allowExcessArguments).toBeTruthy();
-    source.allowExcessArguments(false);
-    cmd.copyInheritedSettings(source);
     expect(cmd._allowExcessArguments).toBeFalsy();
+    source.allowExcessArguments();
+    cmd.copyInheritedSettings(source);
+    expect(cmd._allowExcessArguments).toBeTruthy();
   });
 
   test('when copyInheritedSettings then copies enablePositionalOptions()', () => {

tests/command.hook.test.js

@@ -196,7 +196,10 @@ describe('action hooks context', () => {
     program.argument('[arg]').hook('preAction', (thisCommand) => {
       expect(thisCommand.args).toEqual(['sub', 'value']);
     });
-    program.command('sub').action(() => {});
+    program
+      .command('sub')
+      .argument('<arg>')
+      .action(() => {});
     program.parse(['sub', 'value'], { from: 'user' });
   });
 
@@ -206,7 +209,10 @@ describe('action hooks context', () => {
     program.hook('preAction', (thisCommand, actionCommand) => {
       expect(actionCommand.args).toEqual(['value']);
     });
-    program.command('sub').action(() => {});
+    program
+      .command('sub')
+      .argument('<arg>')
+      .action(() => {});
     program.parse(['sub', 'value'], { from: 'user' });
   });
 });

tests/command.parse.test.js

@@ -10,6 +10,7 @@ const commander = require('../');
 describe('.parse() args from', () => {
   test('when no args then use process.argv and app/script/args', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     const hold = process.argv;
     process.argv = 'node script.js user'.split(' ');
     program.parse();
@@ -19,6 +20,7 @@ describe('.parse() args from', () => {
 
   test('when no args and electron properties and not default app then use process.argv and app/args', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     const holdArgv = process.argv;
     process.versions.electron = '1.2.3';
     process.argv = 'node user'.split(' ');
@@ -30,18 +32,21 @@ describe('.parse() args from', () => {
 
   test('when args then app/script/args', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.parse('node script.js user'.split(' '));
     expect(program.args).toEqual(['user']);
   });
 
   test('when args from "node" then app/script/args', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.parse('node script.js user'.split(' '), { from: 'node' });
     expect(program.args).toEqual(['user']);
   });
 
   test('when args from "electron" and not default app then app/args', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     const hold = process.defaultApp;
     process.defaultApp = undefined;
     program.parse('customApp user'.split(' '), { from: 'electron' });
@@ -51,6 +56,7 @@ describe('.parse() args from', () => {
 
   test('when args from "electron" and default app then app/script/args', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     const hold = process.defaultApp;
     process.defaultApp = true;
     program.parse('electron script user'.split(' '), { from: 'electron' });
@@ -60,12 +66,14 @@ describe('.parse() args from', () => {
 
   test('when args from "user" then args', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.parse('user'.split(' '), { from: 'user' });
     expect(program.args).toEqual(['user']);
   });
 
   test('when args from "silly" then throw', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     expect(() => {
       program.parse(['node', 'script.js'], { from: 'silly' });
     }).toThrow();
@@ -75,6 +83,7 @@ describe('.parse() args from', () => {
     'when node execArgv includes %s then app/args',
     (flag) => {
       const program = new commander.Command();
+      program.argument('[args...]');
       const holdExecArgv = process.execArgv;
       const holdArgv = process.argv;
       process.argv = ['node', 'user-arg'];
@@ -91,6 +100,7 @@ describe('.parse() args from', () => {
 describe('return type', () => {
   test('when call .parse then returns program', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.action(() => {});
 
     const result = program.parse(['node', 'test']);
@@ -99,6 +109,7 @@ describe('return type', () => {
 
   test('when await .parseAsync then returns program', async () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.action(() => {});
 
     const result = await program.parseAsync(['node', 'test']);
@@ -109,6 +120,7 @@ describe('return type', () => {
 // Easy mistake to make when writing unit tests
 test('when parse strings instead of array then throw', () => {
   const program = new commander.Command();
+  program.argument('[args...]');
   expect(() => {
     program.parse('node', 'test');
   }).toThrow();
@@ -117,6 +129,7 @@ test('when parse strings instead of array then throw', () => {
 describe('parse parameter is treated as readonly, per TypeScript declaration', () => {
   test('when parse called then parameter does not change', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.option('--debug');
     const original = ['node', '--debug', 'arg'];
     const param = original.slice();
@@ -126,6 +139,7 @@ describe('parse parameter is treated as readonly, per TypeScript declaration', (
 
   test('when parse called and parsed args later changed then parameter does not change', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.option('--debug');
     const original = ['node', '--debug', 'arg'];
     const param = original.slice();
@@ -137,6 +151,7 @@ describe('parse parameter is treated as readonly, per TypeScript declaration', (
 
   test('when parse called and param later changed then parsed args do not change', () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.option('--debug');
     const param = ['node', '--debug', 'arg'];
     program.parse(param);
@@ -151,6 +166,7 @@ describe('parse parameter is treated as readonly, per TypeScript declaration', (
 describe('parseAsync parameter is treated as readonly, per TypeScript declaration', () => {
   test('when parse called then parameter does not change', async () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.option('--debug');
     const original = ['node', '--debug', 'arg'];
     const param = original.slice();
@@ -160,6 +176,7 @@ describe('parseAsync parameter is treated as readonly, per TypeScript declaratio
 
   test('when parseAsync called and parsed args later changed then parameter does not change', async () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.option('--debug');
     const original = ['node', '--debug', 'arg'];
     const param = original.slice();
@@ -171,6 +188,7 @@ describe('parseAsync parameter is treated as readonly, per TypeScript declaratio
 
   test('when parseAsync called and param later changed then parsed args do not change', async () => {
     const program = new commander.Command();
+    program.argument('[args...]');
     program.option('--debug');
     const param = ['node', '--debug', 'arg'];
     await program.parseAsync(param);

tests/command.parseOptions.test.js

@@ -170,7 +170,7 @@ describe('parseOptions', () => {
 describe('parse and program.args', () => {
   test('when program has known flag and operand then option removed and operand returned', () => {
     const program = new commander.Command();
-    program.option('--global-flag');
+    program.option('--global-flag').argument('[arg...]');
     program.parse('node test.js --global-flag arg'.split(' '));
     expect(program.args).toEqual(['arg']);
   });
@@ -180,7 +180,8 @@ describe('parse and program.args', () => {
     program
       .allowUnknownOption()
       .option('--global-flag')
-      .option('--global-value <value>');
+      .option('--global-value <value>')
+      .argument('[arg...]');
     program.parse(
       'node test.js aaa --global-flag bbb --unknown ccc --global-value value'.split(
         ' ',

tests/command.positionalOptions.test.js

@@ -439,6 +439,7 @@ describe('program with allowUnknownOption', () => {
   test('when passThroughOptions and unknown option then arguments from unknown passed through', () => {
     const program = new commander.Command();
     program.passThroughOptions().allowUnknownOption().option('--debug');
+    program.argument('[args...]');
 
     program.parse(['--unknown', '--debug'], { from: 'user' });
     expect(program.args).toEqual(['--unknown', '--debug']);
@@ -447,6 +448,7 @@ describe('program with allowUnknownOption', () => {
   test('when positionalOptions and unknown option then known options then known option parsed', () => {
     const program = new commander.Command();
     program.enablePositionalOptions().allowUnknownOption().option('--debug');
+    program.argument('[args...]');
 
     program.parse(['--unknown', '--debug'], { from: 'user' });
     expect(program.opts().debug).toBe(true);