Switch from yargs to commander

The problems which previously motivated me to switch from commander to yargs have been solved (by the addition of .exitOverride() and .configureOutput()). Commander has more flexibility than yargs for option processing using .on('option'), which can be used for options which are sensitive to ordering or have more complicated handling. It is also much simpler and has less exotic behavior that needs to be disabled (see all the yargs options which were set). Finally, it is about 1/6 the total size. Also, for this project specifically, yargs@17.1.0 broke positional argument parsing due to yargs/yargs#1977. Since the argument is optional, `.demand()` is not appropriate (it's also deprecated). It appears the correct fix would be to add a default command and define the positional argument on that. However, I'm done dealing with yargs breakage, and switching to Commander will a better return on effort. Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
kevinoid · Aug 5, 2021 · 53ecee8 · 53ecee8
1 parent 44437be
commit 53ecee8
Show file tree

Hide file tree

Showing 3 changed files with 97 additions and 127 deletions.
diff --git a/cli.js b/cli.js
@@ -6,39 +6,19 @@
 
 'use strict';
 
-const yargs = require('yargs/yargs');
+const {
+  Command,
+  InvalidOptionArgumentError,
+  Option,
+} = require('commander');
 
 const packageJson = require('./package.json');
 const hubCiStatus = require('.');
 
 // Same --color options as hub(1)
 const colorOptions = ['always', 'never', 'auto'];
 
-function coerceColor(arg) {
-  if (arg === undefined) {
-    return arg;
-  }
-
-  if (arg === true) {
-    // Treat --color without argument as 'always'.
-    return 'always';
-  }
-
-  if (colorOptions.includes(arg)) {
-    return arg;
-  }
-
-  throw new RangeError(
-    `Unrecognized --color argument '${arg}'.  Choices: ${
-      colorOptions.join(', ')}`,
-  );
-}
-
 function coerceWait(arg) {
-  if (arg === undefined) {
-    return arg;
-  }
-
   if (arg === true) {
     // Treat --wait without argument as infinite wait.
     return Infinity;
@@ -47,16 +27,27 @@ function coerceWait(arg) {
   // Note: Don't treat '' as 0 (no wait), since it's more likely user error
   const val = Number(arg);
   if (arg === '' || Number.isNaN(val)) {
-    throw new TypeError(`Invalid number "${arg}"`);
+    throw new InvalidOptionArgumentError(`Invalid number "${arg}"`);
   }
 
   if (val < 0) {
-    throw new RangeError('--wait must not be negative');
+    throw new InvalidOptionArgumentError('--wait must not be negative');
   }
 
   return val;
 }
 
+/** Option parser to count the number of occurrences of the option.
+ *
+ * @private
+ * @param {boolean|string} optarg Argument passed to option (ignored).
+ * @param {number=} previous Previous value of option (counter).
+ * @returns {number} previous + 1.
+ */
+function countOption(optarg, previous) {
+  return (previous || 0) + 1;
+}
+
 /** Options for command entry points.
  *
  * @typedef {{
@@ -112,104 +103,80 @@ function hubCiStatusMain(args, options, callback) {
     args = [];
   }
 
-  const yargsObj = yargs(args)
-    .parserConfiguration({
-      'parse-numbers': false,
-      'parse-positional-numbers': false,
-      'dot-notation': false,
-      'duplicate-arguments-array': false,
-      'flatten-duplicate-arrays': false,
-      'greedy-arrays': false,
-      'strip-aliased': true,
-      'strip-dashed': true,
-    })
-    .usage('Usage: $0 [options] [ref]')
-    .help()
-    .alias('help', 'h')
-    .alias('help', '?')
-    .options('color', {
-      describe: `Colorize verbose output (${colorOptions.join('|')})`,
-      coerce: coerceColor,
-    })
-    .option('quiet', {
-      alias: 'q',
-      describe: 'Print less output',
-      count: true,
+  const command = new Command()
+    .exitOverride()
+    .configureOutput({
+      writeOut: (str) => options.stdout.write(str),
+      writeErr: (str) => options.stderr.write(str),
+      getOutHelpWidth: () => options.stdout.columns,
+      getErrHelpWidth: () => options.stderr.columns,
     })
-    .option('verbose', {
-      alias: 'v',
-      describe: 'Print more output',
-      count: true,
-    })
-    .option('wait', {
-      alias: 'w',
-      describe: 'Retry while combined status is pending'
-        + ' (with optional max time in sec)',
-      defaultDescription: 'Infinity',
-      coerce: coerceWait,
-    })
-    .options('wait-all', {
-      alias: 'W',
-      boolean: true,
-      describe: 'Retry while any status is pending (implies --wait)',
-    })
-    .version(`${packageJson.name} ${packageJson.version}`)
-    .alias('version', 'V')
-    .strict();
-  yargsObj.parse(args, async (errYargs, argOpts, output) => {
-    if (errYargs) {
-      options.stderr.write(`${output || errYargs}\n`);
-      callback(1);
-      return;
-    }
-
-    if (output) {
-      options.stdout.write(`${output}\n`);
-    }
-
-    if (argOpts.help || argOpts.version) {
-      callback(0);
-      return;
-    }
-
-    if (argOpts._.length > 1) {
-      options.stderr.write(
-        `Error: Expected at most 1 argument, got ${argOpts._.length}.\n`,
-      );
-      callback(1);
-      return;
-    }
-
-    const maxTotalMs = argOpts.wait !== undefined ? argOpts.wait * 1000
-      : argOpts.waitAll ? Infinity
+    .arguments('[ref]')
+    .allowExcessArguments(false)
+    // Check for required/excess arguments.
+    // Workaround https://github.com/tj/commander.js/issues/1493
+    .action(() => {})
+    .description('Command description.')
+    .addOption(
+      new Option('--color [when]', 'Colorize verbose output')
+        .choices(colorOptions),
+    )
+    .option('-q, --quiet', 'Print less output', countOption)
+    .option('-v, --verbose', 'Print more output', countOption)
+    .option(
+      '-w, --wait [seconds]',
+      'Retry while combined status is pending (with optional max time in sec)',
+      coerceWait,
+    )
+    .option(
+      '-W, --wait-all',
+      'Retry while any status is pending (implies --wait)',
+    )
+    .version(packageJson.version);
+
+  try {
+    command.parse(args);
+  } catch (errParse) {
+    const exitCode =
+      errParse.exitCode !== undefined ? errParse.exitCode : 1;
+    process.nextTick(callback, exitCode);
+    return;
+  }
+
+  const argOpts = command.opts();
+  const maxTotalMs =
+    typeof argOpts.wait === 'number' ? argOpts.wait * 1000
+      : argOpts.wait || argOpts.waitAll ? Infinity
         : undefined;
-    const useColor = argOpts.color === 'never' ? false
-      : argOpts.color === 'always' ? true
+  const useColor =
+    argOpts.color === 'never' ? false
+      : argOpts.color === 'always' || argOpts.color === true ? true
         : undefined;
-    const ref = argOpts._[0];
-    const verbosity = (argOpts.verbose || 0) - (argOpts.quiet || 0);
-
-    let exitCode = 0;
-    try {
-      const gcs = options.hubCiStatus || hubCiStatus;
-      exitCode = await gcs(ref, {
-        octokitOptions: {
-          auth: options.env ? options.env.GITHUB_TOKEN : undefined,
-        },
-        stderr: options.stderr,
-        stdout: options.stdout,
-        useColor,
-        verbosity,
-        wait: maxTotalMs === undefined ? undefined : { maxTotalMs },
-        waitAll: !!argOpts.waitAll,
-      });
-    } catch (err) {
-      exitCode = 1;
-      options.stderr.write(`${verbosity > 1 ? err.stack : err}\n`);
-    }
-
-    callback(exitCode);
-  });
+  const ref = command.args[0];
+  const verbosity = (argOpts.verbose || 0) - (argOpts.quiet || 0);
+
+  const gcs = options.hubCiStatus || hubCiStatus;
+  // eslint-disable-next-line promise/catch-or-return
+  gcs(ref, {
+    octokitOptions: {
+      auth: options.env ? options.env.GITHUB_TOKEN : undefined,
+    },
+    stderr: options.stderr,
+    stdout: options.stdout,
+    useColor,
+    verbosity,
+    wait: maxTotalMs === undefined ? undefined : { maxTotalMs },
+    waitAll: !!argOpts.waitAll,
+  })
+    .then(
+      () => 0,
+      (err) => {
+        options.stderr.write(`${verbosity > 1 ? err.stack : err}\n`);
+        return 1;
+      },
+    )
+    // Note: nextTick for unhandledException (like util.callbackify)
+    .then((exitCode) => process.nextTick(callback, exitCode));
 }
 
 module.exports = hubCiStatusMain;
diff --git a/package.json b/package.json
@@ -55,7 +55,7 @@
   },
   "dependencies": {
     "@octokit/rest": "^18.0.12",
-    "yargs": "^17.0.1"
+    "commander": "^7.0.0"
   },
   "devDependencies": {
     "@kevinoid/assert-shim": "^0.1.0",

diff --git a/test/cli.js b/test/cli.js
@@ -100,7 +100,7 @@ describe('hub-ci-status command', () => {
     );
   });
 
-  for (const helpOpt of ['--help', '-h', '-?']) {
+  for (const helpOpt of ['--help', '-h']) {
     it(`${helpOpt} prints help message to stdout`, async () => {
       const args = [...RUNTIME_ARGS, helpOpt];
       const options = getTestOptions();
@@ -121,7 +121,7 @@ describe('hub-ci-status command', () => {
       assert.strictEqual(exitCode, 0);
       assert.strictEqual(options.stderr.read(), null);
       const output = options.stdout.read();
-      assert.strictEqual(output, `hub-ci-status ${packageJson.version}\n`);
+      assert.strictEqual(output, `${packageJson.version}\n`);
     });
   }
 
@@ -289,10 +289,13 @@ describe('hub-ci-status command', () => {
   expectArgsErr(['--wait=nope'], /\bwait\b/);
   expectArgsErr(['--wait='], /\bwait\b/);
   expectArgsErr(['--wait=-1'], /\bwait\b/);
-  expectArgsErr(['--wait', '-1'], /\bwait\b/);
   expectArgsErr(['-wnope'], /\bwait\b/);
   expectArgsErr(['-w-1'], /\bwait\b/);
-  expectArgsErr(['-w', '-1'], /\bwait\b/);
+  // Note: commander treats negative values for optional arguments as unknown
+  // https://github.com/tj/commander.js/issues/61
+  // https://github.com/tj/commander.js/pull/583#issuecomment-486819992
+  expectArgsErr(['--wait', '-1'], /\bunknown option\b/);
+  expectArgsErr(['-w', '-1'], /\bunknown option\b/);
   expectArgsErr(['--unknown123'], /\bunknown123\b/);
   // Note: Differs from hub(1), which ignores unexpected ci-status arguments.
   expectArgsErr(['ref1', 'ref2'], /\barguments?\b/i);