feat!: switch type:string option arguments to greedy, but with error for suspect cases in strict mode (#88)

Author: shadowspawn

index.js

@@ -28,6 +28,7 @@ const {
   isLoneShortOption,
   isLongOptionAndValue,
   isOptionValue,
+  isOptionLikeValue,
   isShortOptionAndValue,
   isShortOptionGroup,
   objectGetOwn,
@@ -77,6 +78,27 @@ function getMainArgs() {
   return ArrayPrototypeSlice(process.argv, 2);
 }
 
+/**
+ * In strict mode, throw for possible usage errors like --foo --bar
+ *
+ * @param {string} longOption - long option name e.g. 'foo'
+ * @param {string|undefined} optionValue - value from user args
+ * @param {string} shortOrLong - option used, with dashes e.g. `-l` or `--long`
+ * @param {boolean} strict - show errors, from parseArgs({ strict })
+ */
+function checkOptionLikeValue(longOption, optionValue, shortOrLong, strict) {
+  if (strict && isOptionLikeValue(optionValue)) {
+    // Only show short example if user used short option.
+    const example = (shortOrLong.length === 2) ?
+      `'--${longOption}=-XYZ' or '${shortOrLong}-XYZ'` :
+      `'--${longOption}=-XYZ'`;
+    const errorMessage = `Option '${shortOrLong}' argument is ambiguous.
+Did you forget to specify the option argument for '${shortOrLong}'?
+Or to specify an option argument starting with a dash use ${example}.`;
+    throw new ERR_PARSE_ARGS_INVALID_OPTION_VALUE(errorMessage);
+  }
+}
+
 /**
  * In strict mode, throw for usage errors.
  *
@@ -210,6 +232,7 @@ const parseArgs = (config = { __proto__: null }) => {
           isOptionValue(nextArg)) {
         // e.g. '-f', 'bar'
         optionValue = ArrayPrototypeShift(remainingArgs);
+        checkOptionLikeValue(longOption, optionValue, arg, strict);
       }
       checkOptionUsage(longOption, optionValue, options,
                        arg, strict, allowPositionals);
@@ -256,6 +279,7 @@ const parseArgs = (config = { __proto__: null }) => {
           isOptionValue(nextArg)) {
         // e.g. '--foo', 'bar'
         optionValue = ArrayPrototypeShift(remainingArgs);
+        checkOptionLikeValue(longOption, optionValue, arg, strict);
       }
       checkOptionUsage(longOption, optionValue, options,
                        arg, strict, allowPositionals);

test/index.js

@@ -436,3 +436,137 @@ test('null prototype: when --toString then values.toString is true', () => {
   const result = parseArgs({ args, options });
   assert.deepStrictEqual(result, expectedResult);
 });
+
+const candidateGreedyOptions = [
+  '',
+  '-',
+  '--',
+  'abc',
+  '123',
+  '-s',
+  '--foo',
+];
+
+candidateGreedyOptions.forEach((value) => {
+  test(`greedy: when short option with value '${value}' then eaten`, () => {
+    const args = ['-w', value];
+    const options = { with: { type: 'string', short: 'w' } };
+    const expectedResult = { values: { __proto__: null, with: value }, positionals: [] };
+
+    const result = parseArgs({ args, options, strict: false });
+    assert.deepStrictEqual(result, expectedResult);
+  });
+
+  test(`greedy: when long option with value '${value}' then eaten`, () => {
+    const args = ['--with', value];
+    const options = { with: { type: 'string', short: 'w' } };
+    const expectedResult = { values: { __proto__: null, with: value }, positionals: [] };
+
+    const result = parseArgs({ args, options, strict: false });
+    assert.deepStrictEqual(result, expectedResult);
+  });
+});
+
+test('strict: when candidate option value is plain text then does not throw', () => {
+  const args = ['--with', 'abc'];
+  const options = { with: { type: 'string' } };
+  const expectedResult = { values: { __proto__: null, with: 'abc' }, positionals: [] };
+
+  const result = parseArgs({ args, options, strict: true });
+  assert.deepStrictEqual(result, expectedResult);
+});
+
+test("strict: when candidate option value is '-' then does not throw", () => {
+  const args = ['--with', '-'];
+  const options = { with: { type: 'string' } };
+  const expectedResult = { values: { __proto__: null, with: '-' }, positionals: [] };
+
+  const result = parseArgs({ args, options, strict: true });
+  assert.deepStrictEqual(result, expectedResult);
+});
+
+test("strict: when candidate option value is '--' then throws", () => {
+  const args = ['--with', '--'];
+  const options = { with: { type: 'string' } };
+
+  assert.throws(() => {
+    parseArgs({ args, options });
+  }, {
+    code: 'ERR_PARSE_ARGS_INVALID_OPTION_VALUE'
+  });
+});
+
+test('strict: when candidate option value is short option then throws', () => {
+  const args = ['--with', '-a'];
+  const options = { with: { type: 'string' } };
+
+  assert.throws(() => {
+    parseArgs({ args, options });
+  }, {
+    code: 'ERR_PARSE_ARGS_INVALID_OPTION_VALUE'
+  });
+});
+
+test('strict: when candidate option value is short option digit then throws', () => {
+  const args = ['--with', '-1'];
+  const options = { with: { type: 'string' } };
+
+  assert.throws(() => {
+    parseArgs({ args, options });
+  }, {
+    code: 'ERR_PARSE_ARGS_INVALID_OPTION_VALUE'
+  });
+});
+
+test('strict: when candidate option value is long option then throws', () => {
+  const args = ['--with', '--foo'];
+  const options = { with: { type: 'string' } };
+
+  assert.throws(() => {
+    parseArgs({ args, options });
+  }, {
+    code: 'ERR_PARSE_ARGS_INVALID_OPTION_VALUE'
+  });
+});
+
+test('strict: when short option and suspect value then throws with short option in error message', () => {
+  const args = ['-w', '--foo'];
+  const options = { with: { type: 'string', short: 'w' } };
+
+  assert.throws(() => {
+    parseArgs({ args, options });
+  }, /for '-w'/
+  );
+});
+
+test('strict: when long option and suspect value then throws with long option in error message', () => {
+  const args = ['--with', '--foo'];
+  const options = { with: { type: 'string' } };
+
+  assert.throws(() => {
+    parseArgs({ args, options });
+  }, /for '--with'/
+  );
+});
+
+test('strict: when short option and suspect value then throws with whole expected message', () => {
+  const args = ['-w', '--foo'];
+  const options = { with: { type: 'string', short: 'w' } };
+
+  assert.throws(() => {
+    parseArgs({ args, options });
+  // eslint-disable-next-line max-len
+  }, /Error: Option '-w' argument is ambiguous\.\nDid you forget to specify the option argument for '-w'\?\nOr to specify an option argument starting with a dash use '--with=-XYZ' or '-w-XYZ'\./
+  );
+});
+
+test('strict: when long option and suspect value then throws with whole expected message', () => {
+  const args = ['--with', '--foo'];
+  const options = { with: { type: 'string', short: 'w' } };
+
+  assert.throws(() => {
+    parseArgs({ args, options });
+  // eslint-disable-next-line max-len
+  }, /Error: Option '--with' argument is ambiguous\.\nDid you forget to specify the option argument for '--with'\?\nOr to specify an option argument starting with a dash use '--with=-XYZ'\./
+  );
+});

test/is-option-like-value.js

@@ -0,0 +1,70 @@
+'use strict';
+/* eslint max-len: 0 */
+
+const test = require('tape');
+const { isOptionLikeValue } = require('../utils.js');
+
+// Basically rejecting values starting with a dash, but run through the interesting possibilities.
+
+test('isOptionLikeValue: when passed plain text then returns false', (t) => {
+  t.false(isOptionLikeValue('abc'));
+  t.end();
+});
+
+test('isOptionLikeValue: when passed digits then returns false', (t) => {
+  t.false(isOptionLikeValue(123));
+  t.end();
+});
+
+test('isOptionLikeValue: when passed empty string then returns false', (t) => {
+  t.false(isOptionLikeValue(''));
+  t.end();
+});
+
+// Special case, used as stdin/stdout et al and not reason to reject
+test('isOptionLikeValue: when passed dash then returns false', (t) => {
+  t.false(isOptionLikeValue('-'));
+  t.end();
+});
+
+test('isOptionLikeValue: when passed -- then returns true', (t) => {
+  // Not strictly option-like, but is supect
+  t.true(isOptionLikeValue('--'));
+  t.end();
+});
+
+// Supporting undefined so can pass element off end of array without checking
+test('isOptionLikeValue: when passed undefined then returns false', (t) => {
+  t.false(isOptionLikeValue(undefined));
+  t.end();
+});
+
+test('isOptionLikeValue: when passed short option then returns true', (t) => {
+  t.true(isOptionLikeValue('-a'));
+  t.end();
+});
+
+test('isOptionLikeValue: when passed short option digit then returns true', (t) => {
+  t.true(isOptionLikeValue('-1'));
+  t.end();
+});
+
+test('isOptionLikeValue: when passed negative number then returns true', (t) => {
+  t.true(isOptionLikeValue('-123'));
+  t.end();
+});
+
+test('isOptionLikeValue: when passed short option group of short option with value then returns true', (t) => {
+  t.true(isOptionLikeValue('-abd'));
+  t.end();
+});
+
+test('isOptionLikeValue: when passed long option then returns true', (t) => {
+  t.true(isOptionLikeValue('--foo'));
+  t.end();
+});
+
+test('isOptionLikeValue: when passed long option with value then returns true', (t) => {
+  t.true(isOptionLikeValue('--foo=bar'));
+  t.end();
+});

test/is-option-value.js

@@ -4,6 +4,8 @@
 const test = require('tape');
 const { isOptionValue } = require('../utils.js');
 
+// Options are greedy so simple behaviour, but run through the interesting possibilities.
+
 test('isOptionValue: when passed plain text then returns true', (t) => {
   t.true(isOptionValue('abc'));
   t.end();
@@ -25,28 +27,43 @@ test('isOptionValue: when passed dash then returns true', (t) => {
   t.end();
 });
 
-// Supporting undefined so can pass element off end of array without checking
+test('isOptionValue: when passed -- then returns true', (t) => {
+  t.true(isOptionValue('--'));
+  t.end();
+});
+
+// Checking undefined so can pass element off end of array.
 test('isOptionValue: when passed undefined then returns false', (t) => {
   t.false(isOptionValue(undefined));
   t.end();
 });
 
-test('isOptionValue: when passed short option then returns false', (t) => {
-  t.false(isOptionValue('-a'));
+test('isOptionValue: when passed short option then returns true', (t) => {
+  t.true(isOptionValue('-a'));
+  t.end();
+});
+
+test('isOptionValue: when passed short option digit then returns true', (t) => {
+  t.true(isOptionValue('-1'));
+  t.end();
+});
+
+test('isOptionValue: when passed negative number then returns true', (t) => {
+  t.true(isOptionValue('-123'));
   t.end();
 });
 
-test('isOptionValue: when passed short option group of short option with value then returns false', (t) => {
-  t.false(isOptionValue('-abd'));
+test('isOptionValue: when passed short option group of short option with value then returns true', (t) => {
+  t.true(isOptionValue('-abd'));
   t.end();
 });
 
-test('isOptionValue: when passed long option then returns false', (t) => {
-  t.false(isOptionValue('--foo'));
+test('isOptionValue: when passed long option then returns true', (t) => {
+  t.true(isOptionValue('--foo'));
   t.end();
 });
 
-test('isOptionValue: when passed long option with value then returns false', (t) => {
-  t.false(isOptionValue('--foo=bar'));
+test('isOptionValue: when passed long option with value then returns true', (t) => {
+  t.true(isOptionValue('--foo=bar'));
   t.end();
 });

utils.js

@@ -39,24 +39,29 @@ function optionsGetOwn(options, longOption, prop) {
 
 /**
  * Determines if the argument may 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('-v') // returns true (greedy)
+ * isOptionValue('--foo') // returns true (greedy)
  * isOptionValue(undefined) // returns false
  */
 function isOptionValue(value) {
   if (value == null) return false;
-  if (value === '-') return true; // e.g. representing stdin/stdout for file
 
   // Open Group Utility Conventions are that an option-argument
   // is the argument after the option, and may start with a dash.
-  // However, we are currently rejecting these and prioritising the
-  // option-like appearance of the argument. Rejection allows more error
-  // detection for strict:true, but comes at the cost of rejecting intended
-  // values starting with a dash, especially negative numbers.
-  return !StringPrototypeStartsWith(value, '-');
+  return true; // greedy!
+}
+
+/**
+ * Detect whether there is possible confusion and user may have omitted
+ * the option argument, like `--port --verbose` when `port` of type:string.
+ * In strict mode we throw errors if value is option-like.
+ */
+function isOptionLikeValue(value) {
+  if (value == null) return false;
+
+  return value.length > 1 && StringPrototypeCharAt(value, 0) === '-';
 }
 
 /**
@@ -172,6 +177,7 @@ module.exports = {
   isLoneShortOption,
   isLongOptionAndValue,
   isOptionValue,
+  isOptionLikeValue,
   isShortOptionAndValue,
   isShortOptionGroup,
   objectGetOwn,