diff --git a/index.js b/index.js index 1d19365..90a1bf5 100644 --- a/index.js +++ b/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); diff --git a/test/index.js b/test/index.js index 68c0de0..d7d7109 100644 --- a/test/index.js +++ b/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'\./ + ); +}); diff --git a/test/is-option-like-value.js b/test/is-option-like-value.js new file mode 100644 index 0000000..9d4ee4f --- /dev/null +++ b/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(); +}); diff --git a/test/is-option-value.js b/test/is-option-value.js index 199bf30..3e7b00a 100644 --- a/test/is-option-value.js +++ b/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(); }); diff --git a/utils.js b/utils.js index 739f49d..c9cabef 100644 --- a/utils.js +++ b/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,