refactor!: parsing, revisit short option groups, add support for combined short and value

.editorconfig

@@ -0,0 +1,12 @@
+# EditorConfig is awesome: http://EditorConfig.org
+
+# top-most EditorConfig file
+root = true
+
+[*]
+end_of_line = lf
+insert_final_newline = true
+indent_style = space
+indent_size = 2
+tab_width = 2
+# trim_trailing_whitespace = true

index.js

@@ -2,18 +2,16 @@
 
 const {
   ArrayPrototypeConcat,
-  ArrayPrototypeFind,
   ArrayPrototypeForEach,
+  ArrayPrototypeShift,
   ArrayPrototypeSlice,
-  ArrayPrototypeSplice,
   ArrayPrototypePush,
   ObjectHasOwn,
   ObjectEntries,
   StringPrototypeCharAt,
   StringPrototypeIncludes,
   StringPrototypeIndexOf,
   StringPrototypeSlice,
-  StringPrototypeStartsWith,
 } = require('./primordials');
 
 const {
@@ -24,6 +22,16 @@ const {
   validateBoolean,
 } = require('./validators');
 
+const {
+  findLongOptionForShort,
+  isLoneLongOption,
+  isLoneShortOption,
+  isLongOptionAndValue,
+  isOptionValue,
+  isShortOptionAndValue,
+  isShortOptionGroup
+} = require('./utils');
+
 function getMainArgs() {
   // This function is a placeholder for proposed process.mainArgs.
   // Work out where to slice process.argv for user supplied arguments.
@@ -116,86 +124,89 @@ const parseArgs = ({
     positionals: []
   };
 
-  let pos = 0;
-  while (pos < args.length) {
-    let arg = args[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(args, ++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 shortOption = StringPrototypeCharAt(arg, i);
-            // Add 'i' to 'pos' such that short options are parsed in order
-            // of definition:
-            ArrayPrototypeSplice(args, pos + (i - 1), 0, `-${shortOption}`);
-          }
-        }
+  let remainingArgs = ArrayPrototypeSlice(args);
+  while (remainingArgs.length > 0) {
+    const arg = ArrayPrototypeShift(remainingArgs);
+    const nextArg = remainingArgs[0];
+
+    // 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,
+        remainingArgs
+      );
+      break; // Finished processing args, leave while loop.
+    }
 
-        arg = StringPrototypeCharAt(arg, 1); // short
+    if (isLoneShortOption(arg)) {
+      // e.g. '-f'
+      const shortOption = StringPrototypeCharAt(arg, 1);
+      const longOption = findLongOptionForShort(shortOption, options);
+      let optionValue;
+      if (options[longOption]?.type === 'string' && isOptionValue(nextArg)) {
+        // e.g. '-f', 'bar'
+        optionValue = ArrayPrototypeShift(remainingArgs);
+      }
+      storeOptionValue(options, longOption, optionValue, result);
+      continue;
+    }
 
-        const [longOption] = ArrayPrototypeFind(
-          ObjectEntries(options),
-          ([, optionConfig]) => optionConfig.short === arg
-        ) || [];
+    if (isShortOptionGroup(arg, options)) {
+      // Expand -fXzy to -f -X -z -y
+      const expanded = [];
+      for (let index = 1; index < arg.length; index++) {
+        const shortOption = StringPrototypeCharAt(arg, index);
+        const longOption = findLongOptionForShort(shortOption, options);
+        if (options[longOption]?.type !== 'string' ||
+          index === arg.length - 1) {
+          // Boolean option, or last short in group. Well formed.
+          ArrayPrototypePush(expanded, `-${shortOption}`);
+        } else {
+          // String option in middle. Yuck.
+          // ToDo: if strict then throw
+          // Expand -abfFILE to -a -b -fFILE
+          ArrayPrototypePush(expanded, `-${StringPrototypeSlice(arg, index)}`);
+          break; // finished short group
+        }
+      }
+      remainingArgs = ArrayPrototypeConcat(expanded, remainingArgs);
+      continue;
+    }
 
-        arg = longOption ?? arg;
+    if (isShortOptionAndValue(arg, options)) {
+      // e.g. -fFILE
+      const shortOption = StringPrototypeCharAt(arg, 1);
+      const longOption = findLongOptionForShort(shortOption, options);
+      const optionValue = StringPrototypeSlice(arg, 2);
+      storeOptionValue(options, longOption, optionValue, result);
+      continue;
+    }
 
-        // ToDo: later code tests for `=` in arg and wrong for shorts
-      } else {
-        arg = StringPrototypeSlice(arg, 2); // remove leading --
+    if (isLoneLongOption(arg)) {
+      // e.g. '--foo'
+      const longOption = StringPrototypeSlice(arg, 2);
+      let optionValue;
+      if (options[longOption]?.type === 'string' && isOptionValue(nextArg)) {
+        // e.g. '--foo', 'bar'
+        optionValue = ArrayPrototypeShift(remainingArgs);
       }
+      storeOptionValue(options, longOption, optionValue, result);
+      continue;
+    }
 
-      if (StringPrototypeIncludes(arg, '=')) {
-        // Store option=value same way independent of `type: "string"` as:
-        // - looks like a value, store as a value
-        // - match the intention of the user
-        // - preserve information for author to process further
-        const index = StringPrototypeIndexOf(arg, '=');
-        storeOptionValue(
-          options,
-          StringPrototypeSlice(arg, 0, index),
-          StringPrototypeSlice(arg, index + 1),
-          result);
-      } else if (pos + 1 < args.length &&
-        !StringPrototypeStartsWith(args[pos + 1], '-')
-      ) {
-        // `type: "string"` option should also support setting values when '='
-        // isn't used ie. both --foo=b and --foo b should work
-
-        // If `type: "string"` 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[arg] && options[arg].type === 'string' ?
-          args[++pos] :
-          undefined;
-        storeOptionValue(options, arg, val, result);
-      } 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);
-      }
-    } else {
-      // Arguments without a dash prefix are considered "positional"
-      ArrayPrototypePush(result.positionals, arg);
+    if (isLongOptionAndValue(arg)) {
+      // e.g. --foo=bar
+      const index = StringPrototypeIndexOf(arg, '=');
+      const longOption = StringPrototypeSlice(arg, 2, index);
+      const optionValue = StringPrototypeSlice(arg, index + 1);
+      storeOptionValue(options, longOption, optionValue, result);
+      continue;
     }
 
-    pos++;
+    // Anything left is a positional
+    ArrayPrototypePush(result.positionals, arg);
   }
 
   return result;

package.json

@@ -3,9 +3,13 @@
   "version": "0.3.0",
   "description": "Polyfill of future proposal for `util.parseArgs()`",
   "main": "index.js",
+  "exports": {
+    ".": "./index.js",
+    "./package.json": "./package.json"
+  },
   "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,34 @@
+'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
+//
+// A different usage and example is `git switch -` to switch back to the previous branch.
+
+test("dash: when args include '-' used as positional then result has '-' in positionals", (t) => {
+  const passedArgs = ['-'];
+  const expected = { flags: {}, values: {}, positionals: ['-'] };
+
+  const result = parseArgs({ args: passedArgs });
+
+  t.deepEqual(result, expected);
+  t.end();
+});
+
+// If '-' is a valid positional, it is symmetrical to allow it as an option value too.
+test("dash: when args include '-' used as space-separated option value then result has '-' in option value", (t) => {
+  const passedArgs = ['-v', '-'];
+  const passedOptions = { v: { type: 'string' } };
+  const expected = { flags: { v: true }, values: { v: '-' }, positionals: [] };
+
+  const result = parseArgs({ args: passedArgs, options: passedOptions });
+
+  t.deepEqual(result, expected);
+  t.end();
+});

test/find-long-option-for-short.js

@@ -0,0 +1,20 @@
+'use strict';
+/* eslint max-len: 0 */
+
+const test = require('tape');
+const { findLongOptionForShort } = require('../utils.js');
+
+test('findLongOptionForShort: when passed empty options then returns short', (t) => {
+  t.equal(findLongOptionForShort('a', {}), 'a');
+  t.end();
+});
+
+test('findLongOptionForShort: when passed short not present in options then returns short', (t) => {
+  t.equal(findLongOptionForShort('a', { foo: { short: 'f', type: 'string' } }), 'a');
+  t.end();
+});
+
+test('findLongOptionForShort: when passed short present in options then returns long', (t) => {
+  t.equal(findLongOptionForShort('a', { alpha: { short: 'a' } }), 'alpha');
+  t.end();
+});

test/is-lone-long-option.js

@@ -0,0 +1,62 @@
+'use strict';
+/* eslint max-len: 0 */
+
+const test = require('tape');
+const { isLoneLongOption } = require('../utils.js');
+
+test('isLoneLongOption: when passed short option then returns false', (t) => {
+  t.false(isLoneLongOption('-s'));
+  t.end();
+});
+
+test('isLoneLongOption: when passed short option group then returns false', (t) => {
+  t.false(isLoneLongOption('-abc'));
+  t.end();
+});
+
+test('isLoneLongOption: when passed lone long option then returns true', (t) => {
+  t.true(isLoneLongOption('--foo'));
+  t.end();
+});
+
+test('isLoneLongOption: when passed single character long option then returns true', (t) => {
+  t.true(isLoneLongOption('--f'));
+  t.end();
+});
+
+test('isLoneLongOption: when passed long option and value then returns false', (t) => {
+  t.false(isLoneLongOption('--foo=bar'));
+  t.end();
+});
+
+test('isLoneLongOption: when passed empty string then returns false', (t) => {
+  t.false(isLoneLongOption(''));
+  t.end();
+});
+
+test('isLoneLongOption: when passed plain text then returns false', (t) => {
+  t.false(isLoneLongOption('foo'));
+  t.end();
+});
+
+test('isLoneLongOption: when passed single dash then returns false', (t) => {
+  t.false(isLoneLongOption('-'));
+  t.end();
+});
+
+test('isLoneLongOption: when passed double dash then returns false', (t) => {
+  t.false(isLoneLongOption('--'));
+  t.end();
+});
+
+// This is a bit bogus, but simple consistent behaviour: long option follows double dash.
+test('isLoneLongOption: when passed arg starting with triple dash then returns true', (t) => {
+  t.true(isLoneLongOption('---foo'));
+  t.end();
+});
+
+// This is a bit bogus, but simple consistent behaviour: long option follows double dash.
+test("isLoneLongOption: when passed '--=' then returns true", (t) => {
+  t.true(isLoneLongOption('--='));
+  t.end();
+});

test/is-lone-short-option.js

@@ -0,0 +1,45 @@
+'use strict';
+/* eslint max-len: 0 */
+
+const test = require('tape');
+const { isLoneShortOption } = require('../utils.js');
+
+test('isLoneShortOption: when passed short option then returns true', (t) => {
+  t.true(isLoneShortOption('-s'));
+  t.end();
+});
+
+test('isLoneShortOption: when passed short option group (or might be short and value) then returns false', (t) => {
+  t.false(isLoneShortOption('-abc'));
+  t.end();
+});
+
+test('isLoneShortOption: when passed long option then returns false', (t) => {
+  t.false(isLoneShortOption('--foo'));
+  t.end();
+});
+
+test('isLoneShortOption: when passed long option with value then returns false', (t) => {
+  t.false(isLoneShortOption('--foo=bar'));
+  t.end();
+});
+
+test('isLoneShortOption: when passed empty string then returns false', (t) => {
+  t.false(isLoneShortOption(''));
+  t.end();
+});
+
+test('isLoneShortOption: when passed plain text then returns false', (t) => {
+  t.false(isLoneShortOption('foo'));
+  t.end();
+});
+
+test('isLoneShortOption: when passed single dash then returns false', (t) => {
+  t.false(isLoneShortOption('-'));
+  t.end();
+});
+
+test('isLoneShortOption: when passed double dash then returns false', (t) => {
+  t.false(isLoneShortOption('--'));
+  t.end();
+});