Throw for author errors and improve (#1165)

* Throw errors for author errors. Detect bad variadic when added. * Throw errors for author errors. Detect bad variadic when added.
tj · Feb 5, 2020 · d9ac483 · d9ac483
1 parent cb84f6e
commit d9ac483
Show file tree

Hide file tree

Showing 4 changed files with 18 additions and 78 deletions.
diff --git a/index.js b/index.js
@@ -196,7 +196,7 @@ class Command extends EventEmitter {
    */
 
   addCommand(cmd) {
-    if (!cmd._name) throw new Error('Command passed to .AddCommand must have a name');
+    if (!cmd._name) throw new Error('Command passed to .addCommand() must have a name');
 
     // To keep things simple, block automatic name generation for deeply nested executables.
     // Fail fast and detect when adding rather than later when parsing.
@@ -300,6 +300,11 @@ class Command extends EventEmitter {
         self._args.push(argDetails);
       }
     });
+    this._args.forEach(function(arg, i) {
+      if (arg.variadic && i < self._args.length - 1) {
+        throw new Error(`only the last argument can be variadic '${arg.name}'`);
+      }
+    });
     return this;
   };
 
@@ -566,8 +571,7 @@ class Command extends EventEmitter {
   storeOptionsAsProperties(value) {
     this._storeOptionsAsProperties = (value === undefined) || value;
     if (this.options.length) {
-      // This is for programmer, not end user.
-      console.error('Commander usage error: call storeOptionsAsProperties before adding options');
+      throw new Error('call .storeOptionsAsProperties() before adding options');
     }
     return this;
   };
@@ -736,9 +740,12 @@ class Command extends EventEmitter {
     }
     proc.on('error', function(err) {
       if (err.code === 'ENOENT') {
-        console.error('error: %s(1) does not exist', bin);
+        const executableMissing = `'${bin}' does not exist
+ - if '${subcommand._name}' is not meant to be an executable command, remove description parameter from '.command()' and use '.description()' instead
+ - if the default executable name is not suitable, use the executableFile option to supply a custom name`;
+        throw new Error(executableMissing);
       } else if (err.code === 'EACCES') {
-        console.error('error: %s(1) not executable', bin);
+        throw new Error(`'${bin}' not executable`);
       }
       if (!exitCallback) {
         process.exit(1);
@@ -809,10 +816,6 @@ class Command extends EventEmitter {
           if (arg.required && args[i] == null) {
             self.missingArgument(arg.name);
           } else if (arg.variadic) {
-            if (i !== self._args.length - 1) {
-              self.variadicArgNotLast(arg.name);
-            }
-
             args[i] = args.splice(i);
           }
         });
@@ -1068,19 +1071,6 @@ class Command extends EventEmitter {
     this._exit(1, 'commander.unknownCommand', message);
   };
 
-  /**
-   * Variadic argument with `name` is not the last argument as required.
-   *
-   * @param {String} name
-   * @api private
-   */
-
-  variadicArgNotLast(name) {
-    const message = `error: variadic arguments must be last '${name}'`;
-    console.error(message);
-    this._exit(1, 'commander.variadicArgNotLast', message);
-  };
-
   /**
    * Set the program version to `str`.
    *

diff --git a/tests/args.variadic.test.js b/tests/args.variadic.test.js
@@ -70,25 +70,17 @@ describe('variadic argument', () => {
 
   test('when program variadic argument not last then error', () => {
     const program = new commander.Command();
-    program
-      .exitOverride()
-      .arguments('<variadicArg...> [optionalArg]')
-      .action(jest.fn);
 
     expect(() => {
-      program.parse(['node', 'test', 'a']);
-    }).toThrow("error: variadic arguments must be last 'variadicArg'");
+      program.arguments('<variadicArg...> [optionalArg]');
+    }).toThrow("only the last argument can be variadic 'variadicArg'");
   });
 
   test('when command variadic argument not last then error', () => {
     const program = new commander.Command();
-    program
-      .exitOverride()
-      .command('sub <variadicArg...> [optionalArg]')
-      .action(jest.fn);
 
     expect(() => {
-      program.parse(['node', 'test', 'sub', 'a']);
-    }).toThrow("error: variadic arguments must be last 'variadicArg'");
+      program.command('sub <variadicArg...> [optionalArg]');
+    }).toThrow("only the last argument can be variadic 'variadicArg'");
   });
 });
diff --git a/tests/command.executableSubcommand.lookup.test.js b/tests/command.executableSubcommand.lookup.test.js
@@ -12,7 +12,7 @@ test('when subcommand file missing then error', (done) => {
       // Get uncaught thrown error on Windows
       expect(stderr.length).toBeGreaterThan(0);
     } else {
-      expect(stderr).toBe('error: pm-list(1) does not exist\n');
+      expect(stderr).toMatch(new RegExp(/Error: 'pm-list' does not exist/));
     }
     done();
   });
@@ -24,7 +24,7 @@ test('when alias subcommand file missing then error', (done) => {
       // Get uncaught thrown error on Windows
       expect(stderr.length).toBeGreaterThan(0);
     } else {
-      expect(stderr).toBe('error: pm-list(1) does not exist\n');
+      expect(stderr).toMatch(new RegExp(/Error: 'pm-list' does not exist/));
     }
     done();
   });

diff --git a/tests/command.exitOverride.test.js b/tests/command.exitOverride.test.js
@@ -13,8 +13,6 @@ function expectCommanderError(err, exitCode, code, message) {
   expect(err.message).toBe(message);
 }
 
-const testOrSkipOnWindows = (process.platform === 'win32') ? test.skip : test;
-
 describe('.exitOverride and error details', () => {
   // Use internal knowledge to suppress output to keep test output clean.
   let consoleErrorSpy;
@@ -174,25 +172,6 @@ describe('.exitOverride and error details', () => {
     expectCommanderError(caughtErr, 0, 'commander.version', myVersion);
   });
 
-  test('when program variadic argument not last then throw CommanderError', () => {
-    // Note: this error is notified during parse, although could have been detected at declaration.
-    const program = new commander.Command();
-    program
-      .exitOverride()
-      .arguments('<myVariadicArg...> [optionalArg]')
-      .action(jest.fn);
-
-    let caughtErr;
-    try {
-      program.parse(['node', 'test', 'a']);
-    } catch (err) {
-      caughtErr = err;
-    }
-
-    expect(consoleErrorSpy).toHaveBeenCalled();
-    expectCommanderError(caughtErr, 1, 'commander.variadicArgNotLast', "error: variadic arguments must be last 'myVariadicArg'");
-  });
-
   test('when executableSubcommand succeeds then call exitOverride', (done) => {
     const pm = path.join(__dirname, 'fixtures/pm');
     const program = new commander.Command();
@@ -206,27 +185,6 @@ describe('.exitOverride and error details', () => {
     program.parse(['node', pm, 'silent']);
   });
 
-  // Throws directly on Windows
-  testOrSkipOnWindows('when executableSubcommand fails then call exitOverride', (done) => {
-    // Tricky for override, get called for `error` event then `exit` event.
-    const exitCallback = jest.fn()
-      .mockImplementationOnce((err) => {
-        expectCommanderError(err, 1, 'commander.executeSubCommandAsync', '(error)');
-        expect(err.nestedError.code).toBe('ENOENT');
-      })
-      .mockImplementation((err) => {
-        expectCommanderError(err, 0, 'commander.executeSubCommandAsync', '(close)');
-        done();
-      });
-    const pm = path.join(__dirname, 'fixtures/pm');
-    const program = new commander.Command();
-    program
-      .exitOverride(exitCallback)
-      .command('does-not-exist', 'fail');
-
-    program.parse(['node', pm, 'does-not-exist']);
-  });
-
   test('when mandatory program option missing then throw CommanderError', () => {
     const optionFlags = '-p, --pepper <type>';
     const program = new commander.Command();