refactor!: parsing, revisit short option groups, add support for comb…

…ined short and value (#75)


1) Refactor parsing to use independent blocks of code, rather than nested cascading context. This makes it easier to reason about the behaviour.

2) Split out small pieces of logic to named routines to improve readability, and allow extra documentation and examples without cluttering the parsing. (Thanks to @aaronccasanova for inspiration.)

3) Existing tests untouched to make it clear that the tested functionality has not changed.

4) Be more explicit about short option group expansion, and ready to throw error in strict mode for string option in the middle of the argument. (See #11 and #74.)

5) Add support for short option combined with value (without intervening `=`). This is what Commander and Open Group Utility Conventions do, but is _not_ what Yargs does. I don't want to block PR on this and happy to comment it out for further discussion if needed. (I have found some interesting variations in the wild.) [Edit: see also #78]

6) Add support for multiple unit tests files. Expand tests from 33 to 113, but many for internal routines rather than testing exposed API.

7) Added `.editorconfig` file


Co-authored-by: Jordan Harband <[email protected]>
Co-authored-by: Aaron Casanova <[email protected]>

.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();
+});