Refactor parsing for readability and future expansion, and tighten short option group parsing

index.js

@@ -3,6 +3,7 @@
 const {
   ArrayPrototypeConcat,
   ArrayPrototypeIncludes,
+  ArrayPrototypeMap,
   ArrayPrototypeSlice,
   ArrayPrototypeSplice,
   ArrayPrototypePush,
@@ -53,7 +54,7 @@ function getMainArgs() {
   return ArrayPrototypeSlice(process.argv, 2);
 }
 
-function storeOptionValue(parseOptions, option, value, result) {
+function storeOptionValue(option, value, parseOptions, result) {
   const multiple = parseOptions.multiples &&
     ArrayPrototypeIncludes(parseOptions.multiples, option);
 
@@ -97,84 +98,164 @@ const parseArgs = (
 
   let pos = 0;
   while (pos < argv.length) {
-    let arg = argv[pos];
-
-    if (StringPrototypeStartsWith(arg, '-')) {
-      if (arg === '-') {
-        // '-' commonly used to represent stdin/stdout, treat as positional
-        result.positionals = ArrayPrototypeConcat(result.positionals, '-');
-        ++pos;
-        continue;
-      } else if (arg === '--') {
-        // Everything after a bare '--' is considered a positional argument
-        // and is returned verbatim
-        result.positionals = ArrayPrototypeConcat(
-          result.positionals,
-          ArrayPrototypeSlice(argv, ++pos)
-        );
-        return result;
-      } else if (StringPrototypeCharAt(arg, 1) !== '-') {
-        // Look for shortcodes: -fXzy and expand them to -f -X -z -y:
-        if (arg.length > 2) {
-          for (let i = 2; i < arg.length; i++) {
-            const short = StringPrototypeCharAt(arg, i);
-            // Add 'i' to 'pos' such that short options are parsed in order
-            // of definition:
-            ArrayPrototypeSplice(argv, pos + (i - 1), 0, `-${short}`);
-          }
-        }
+    const arg = argv[pos];
+    const nextArg = argv[pos + 1];
 
-        arg = StringPrototypeCharAt(arg, 1); // short
-        if (options.short && options.short[arg])
-          arg = options.short[arg]; // now long!
-        // ToDo: later code tests for `=` in arg and wrong for shorts
-      } else {
-        arg = StringPrototypeSlice(arg, 2); // remove leading --
+    // Check if `arg` is an options terminator.
+    // Guideline 10 in https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
+    if (arg === '--') {
+      // Everything after a bare '--' is considered a positional argument.
+      result.positionals = ArrayPrototypeConcat(
+        result.positionals,
+        ArrayPrototypeSlice(argv, pos + 1)
+      );
+      break; // Finished processing argv, leave while loop.
+    }
+
+    if (isLoneShortOption(arg)) {
+      // e.g. '-f'
+      const optionKey = getOptionKey(StringPrototypeCharAt(arg, 1), options);
+      let optionValue;
+      if (isExpectingValue(optionKey, options) && isOptionValue(nextArg)) {
+        // e.g. '-f' 'bar'
+        optionValue = nextArg;
+        pos++;
       }
+      storeOptionValue(optionKey, optionValue, options, result);
+      pos++;
+      continue;
+    }
+
+    if (isShortOptionGroup(arg, options)) {
+      // Expand -fXzy to -f -X -z -y
+      const expanded = ArrayPrototypeMap(StringPrototypeSlice(arg, 1), (char) => `-${char}`);
+      // Replace group with expansion.
+      ArrayPrototypeSplice(argv, pos, 1, ...expanded);
+      continue;
+    }
 
+    if (isLongOption(arg)) {
+      let optionKey;
+      let optionValue;
       if (StringPrototypeIncludes(arg, '=')) {
-        // Store option=value same way independent of `withValue` as:
-        // - looks like a value, store as a value
-        // - match the intention of the user
-        // - preserve information for author to process further
+        // e.g. '--foo=bar'
         const index = StringPrototypeIndexOf(arg, '=');
-        storeOptionValue(
-          options,
-          StringPrototypeSlice(arg, 0, index),
-          StringPrototypeSlice(arg, index + 1),
-          result);
-      } else if (pos + 1 < argv.length &&
-        !StringPrototypeStartsWith(argv[pos + 1], '-')
-      ) {
-        // withValue option should also support setting values when '=
-        // isn't used ie. both --foo=b and --foo b should work
-
-        // If withValue option is specified, take next position argument as
-        // value and then increment pos so that we don't re-evaluate that
-        // arg, else set value as undefined ie. --foo b --bar c, after setting
-        // b as the value for foo, evaluate --bar next and skip 'b'
-        const val = options.withValue &&
-          ArrayPrototypeIncludes(options.withValue, arg) ? argv[++pos] :
-          undefined;
-        storeOptionValue(options, arg, val, result);
+        optionKey = StringPrototypeSlice(arg, 2, index);
+        optionValue = StringPrototypeSlice(arg, index + 1);
       } else {
-        // Cases when an arg is specified without a value, example
-        // '--foo --bar' <- 'foo' and 'bar' flags should be set to true and
-        // save value as undefined
-        storeOptionValue(options, arg, undefined, result);
+        // e.g. '--foo'
+        optionKey = StringPrototypeSlice(arg, 2);
+        if (isExpectingValue(optionKey, options) && isOptionValue(nextArg)) {
+          // e.g. '--foo' 'bar'
+          optionValue = nextArg;
+          pos++;
+        }
       }
-
-    } else {
-      // Arguments without a dash prefix are considered "positional"
-      ArrayPrototypePush(result.positionals, arg);
+      storeOptionValue(optionKey, optionValue, options, result);
+      pos++;
+      continue;
     }
 
+    // Anything that did not get handled above is a positional.
+    ArrayPrototypePush(result.positionals, arg);
     pos++;
   }
 
   return result;
 };
 
+/**
+  * Determines if `arg` is a just a short option.
+  * @example '-f'
+  */
+function isLoneShortOption(arg) {
+  return arg.length === 2 &&
+    StringPrototypeCharAt(arg, 0) === '-' &&
+    StringPrototypeCharAt(arg, 1) !== '-';
+}
+
+/**
+  * Determines if `arg` is a short option group.
+  *
+  * See Guideline 5 of the [Open Group Utility Conventions](https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html).
+  *   One or more options without option-arguments, followed by at most one
+  *   option that takes an option-argument, should be accepted when grouped
+  *   behind one '-' delimiter.
+  * @example
+  * isShortOptionGroup('-a', {}) // returns false
+  * isShortOptionGroup('-ab', {}) // returns true
+  * isShortOptionGroup('-fb', { withValue: ['f'] }) // returns false
+  * isShortOptionGroup('-bf', { withValue: ['f'] }) // returns true
+  */
+function isShortOptionGroup(arg, options) {
+  if (arg.length <= 2) return false;
+  if (StringPrototypeCharAt(arg, 0) !== '-') return false;
+  if (StringPrototypeCharAt(arg, 1) === '-') return false;
+
+  const onlyFlags = StringPrototypeSlice(arg, 1, -1);
+  for (let index = 0; index < onlyFlags.length; index++) {
+    const optionKey = getOptionKey(StringPrototypeCharAt(onlyFlags, index));
+    if (isExpectingValue(optionKey, options)) {
+      return false;
+    }
+  }
+  return true;
+}
+
+/**
+  * Determines if `arg` is a long option, which may have a trailing value.
+  * @example
+  * isLongOption('-a) // returns false
+  * isLongOption('--foo) // returns true
+  * isLongOption('--foo=bar) // returns true
+  */
+function isLongOption(arg) {
+  return arg.length > 2 && StringPrototypeStartsWith(arg, '--');
+}
+
+/**
+  * Expand known short options into long, otherwise return original.
+  * @example
+  * getOptionKey('f', { short: { f: 'file'}}) // returns 'file'
+  * getOptionKey('b', {}) // returns 'b'
+  * getOptionKey('long-option', {}) // returns 'long-option'
+  */
+function getOptionKey(option, options) {
+  if (option.length === 1 && options?.short?.[option]) {
+    return options.short[option]; // long option
+  }
+  return option;
+}
+
+/**
+ * Determines if the option is expecting a value.
+ */
+function isExpectingValue(optionKey, options) {
+  return options && options.withValue &&
+    ArrayPrototypeIncludes(options.withValue, optionKey);
+}
+
+/**
+  * Determines if the argument can be used as an option value.
+  * NB: We are choosing not to accept option-ish arguments.
+  * @example
+  * isOptionValue('V']) // returns true
+  * isOptionValue('-v') // returns false
+  * isOptionValue('--foo') // returns false
+  * isOptionValue(undefined) // returns false
+  */
+function isOptionValue(value) {
+  if (value === undefined) return false;
+  if (value === '-') return true; // e.g. representing stdin/stdout for file
+
+  // Open Group Utility Conventions are that an option-argument may start
+  // with a dash, but we are currentlly rejecting these and prioritising the
+  // option-like appearance of the argument. Rejection allows error detection
+  // if strict:true, but comes at the cost of rejecting intended values starting
+  // with a dash, especially negative numbers.
+  return !StringPrototypeStartsWith(value, '-');
+}
+
 module.exports = {
   parseArgs
 };

package.json

@@ -4,8 +4,8 @@
   "description": "Polyfill of future proposal for `util.parseArgs()`",
   "main": "index.js",
   "scripts": {
-    "coverage": "c8 --check-coverage node test/index.js",
-    "test": "c8 node test/index.js",
+    "coverage": "c8 --check-coverage tape 'test/*.js'",
+    "test": "c8 tape 'test/*.js'",
     "posttest": "eslint .",
     "fix": "npm run posttest -- --fix"
   },

test/dash.js

@@ -0,0 +1,31 @@
+'use strict';
+/* eslint max-len: 0 */
+
+const test = require('tape');
+const { parseArgs } = require('../index.js');
+
+// The use of `-` as a positional is specifically mentioned in the Open Group Utility Conventions.
+// The interpretation is up to the utility, and for a file positional (operand) the examples are
+// '-' may stand for standard input (or standard output), or for a file named -.
+// https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
+test("when args include '-' used as positional then result has '-' in positionals", function(t) {
+  const passedArgs = ['-'];
+
+  const args = parseArgs(passedArgs);
+  const expected = { flags: {}, values: {}, positionals: ['-'] };
+  t.deepEqual(args, expected);
+
+  t.end();
+});
+
+// If '-' is a valid positional, it is symmetrical to allow it as an option value too.
+test("when args include '-' used as space-separated option value then result has '-' in option value", function(t) {
+  const passedArgs = ['-v', '-'];
+  const options = { withValue: ['v'] };
+
+  const args = parseArgs(passedArgs, options);
+  const expected = { flags: { v: true }, values: { v: '-' }, positionals: [] };
+  t.deepEqual(args, expected);
+
+  t.end();
+});

test/index.js

@@ -70,58 +70,6 @@ test('when short option withValue used without value then stored as flag', funct
   t.end();
 });
 
-test('short option group behaves like multiple short options', function(t) {
-  const passedArgs = ['-rf'];
-  const passedOptions = { };
-  const expected = { flags: { r: true, f: true }, values: { r: undefined, f: undefined }, positionals: [] };
-  const args = parseArgs(passedArgs, passedOptions);
-
-  t.deepEqual(args, expected);
-
-  t.end();
-});
-
-test('short option group does not consume subsequent positional', function(t) {
-  const passedArgs = ['-rf', 'foo'];
-  const passedOptions = { };
-  const expected = { flags: { r: true, f: true }, values: { r: undefined, f: undefined }, positionals: ['foo'] };
-  const args = parseArgs(passedArgs, passedOptions);
-  t.deepEqual(args, expected);
-
-  t.end();
-});
-
-// See: Guideline 5 https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
-test('if terminal of short-option group configured withValue, subsequent positional is stored', function(t) {
-  const passedArgs = ['-rvf', 'foo'];
-  const passedOptions = { withValue: ['f'] };
-  const expected = { flags: { r: true, f: true, v: true }, values: { r: undefined, v: undefined, f: 'foo' }, positionals: [] };
-  const args = parseArgs(passedArgs, passedOptions);
-  t.deepEqual(args, expected);
-
-  t.end();
-});
-
-test('handles short-option groups in conjunction with long-options', function(t) {
-  const passedArgs = ['-rf', '--foo', 'foo'];
-  const passedOptions = { withValue: ['foo'] };
-  const expected = { flags: { r: true, f: true, foo: true }, values: { r: undefined, f: undefined, foo: 'foo' }, positionals: [] };
-  const args = parseArgs(passedArgs, passedOptions);
-  t.deepEqual(args, expected);
-
-  t.end();
-});
-
-test('handles short-option groups with "short" alias configured', function(t) {
-  const passedArgs = ['-rf'];
-  const passedOptions = { short: { r: 'remove' } };
-  const expected = { flags: { remove: true, f: true }, values: { remove: undefined, f: undefined }, positionals: [] };
-  const args = parseArgs(passedArgs, passedOptions);
-  t.deepEqual(args, expected);
-
-  t.end();
-});
-
 test('Everything after a bare `--` is considered a positional argument', function(t) {
   const passedArgs = ['--', 'barepositionals', 'mopositionals'];
   const expected = { flags: {}, values: {}, positionals: ['barepositionals', 'mopositionals'] };
@@ -163,16 +111,6 @@ test('args equals are passed "withValue"', function(t) {
   t.end();
 });
 
-test('when args include single dash then result stores dash as positional', function(t) {
-  const passedArgs = ['-'];
-  const expected = { flags: { }, values: { }, positionals: ['-'] };
-  const args = parseArgs(passedArgs);
-
-  t.deepEqual(args, expected);
-
-  t.end();
-});
-
 test('zero config args equals are parsed as if "withValue"', function(t) {
   const passedArgs = ['--so=wat'];
   const passedOptions = { };