From 53ecee865784ee66b48303d79107f62c59f6d942 Mon Sep 17 00:00:00 2001 From: Kevin Locke Date: Fri, 30 Apr 2021 09:42:36 -0600 Subject: [PATCH] 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 --- cli.js | 211 ++++++++++++++++++++++----------------------------- package.json | 2 +- test/cli.js | 11 ++- 3 files changed, 97 insertions(+), 127 deletions(-) diff --git a/cli.js b/cli.js index 51910a0..7b8b4f0 100644 --- a/cli.js +++ b/cli.js @@ -6,7 +6,11 @@ 'use strict'; -const yargs = require('yargs/yargs'); +const { + Command, + InvalidOptionArgumentError, + Option, +} = require('commander'); const packageJson = require('./package.json'); const hubCiStatus = require('.'); @@ -14,31 +18,7 @@ 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 index 2662ae1..2e4d0a5 100644 --- 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 index ff3fdb8..1af0a7d 100644 --- 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);