Check command-arguments even without action handler (#1502)

* Check command-arguments even without action handler * Separate checking number of arguments and collecting variadic
tj · Apr 11, 2021 · 0504945 · 0504945
1 parent eade1e0
commit 0504945
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 61 deletions.
diff --git 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
@@ -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('<file>')
-      .exitOverride()
-      .allowExcessArguments(false)
-      .action(() => {});
+      .allowExcessArguments(false);
 
     expect(() => {
       program.parse(['file'], { from: 'user' });
@@ -106,11 +93,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' });
@@ -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
@@ -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('<arg-name>');
+
+    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