Combine normalize and parseoptions, and update combined option handling (#1152)

* Add tests for Utility Conventions before changing code to match

* Switch from preflight including normalise to just testing parseOptions

* Only refactor known options

Do the work in .parseOptions and delete single-pass .normalize

* Add short flag processing to READMEm, and literal --

* Improve character description

* Add a note that options not positional.

* Remove regression tests for bug not really fixed by this

* Add back #561 into known issues

* Refactor to make a little clearer and symmetrical

* Use template to construct strings consistently within parseOptions

Author: shadowspawn

Readme.md

@@ -67,11 +67,18 @@ For larger programs which may use commander in multiple ways, including unit tes
 
 ## Options
 
-Options are defined with the `.option()` method, also serving as documentation for the options. Each option can have a short flag (single character) and a long name, separated by a comma or space.
+Options are defined with the `.option()` method, also serving as documentation for the options. Each option can have a short flag (single character) and a long name, separated by a comma or space or vertical bar ('|').
 
-The options can be accessed as properties on the Command object. Multi-word options such as "--template-engine" are camel-cased, becoming `program.templateEngine` etc. Multiple short flags may be combined as a single arg, for example `-abc` is equivalent to `-a -b -c`.
+The options can be accessed as properties on the Command object. Multi-word options such as "--template-engine" are camel-cased, becoming `program.templateEngine` etc. See also optional new behaviour to [avoid name clashes](#avoiding-option-name-clashes).
 
-See also optional new behaviour to [avoid name clashes](#avoiding-option-name-clashes).
+Multiple short flags may optionally be combined in a single argument following the dash: boolean flags, the last flag may take a value, and the value.
+For example `-a -b -p 80` may be written as `-ab -p80` or even `-abp80`.
+
+You can use `--` to indicate the end of the options, and any remaining arguments will be used without being interpreted.
+This is particularly useful for passing options through to another
+command, like: `do -- git --version`.
+
+Options on the command line are not positional, and can be specified before or after other command arguments.
 
 ### Common option types, boolean and value
 
@@ -251,7 +258,7 @@ $ custom --list x,y,z
 
 ### Required option
 
-You may specify a required (mandatory) option using `.requiredOption`. The option must be specified on the command line, or by having a default value. The method is otherwise the same as `.option` in format, taking flags and description, and optional default value or custom processing.
+You may specify a required (mandatory) option using `.requiredOption`. The option must have a value after parsing, usually specified on the command line, or perhaps from a default value (say from environment). The method is otherwise the same as `.option` in format, taking flags and description, and optional default value or custom processing.
 
 ```js
 const program = require('commander');

index.js

@@ -637,9 +637,8 @@ Command.prototype.parse = function(argv) {
     argv.push(this._helpLongFlag);
   }
 
-  // process argv
-  const normalized = this.normalize(argv.slice(2));
-  const parsed = this.parseOptions(normalized);
+  // process argv, leaving off first two args which are app and scriptname.
+  const parsed = this.parseOptions(argv.slice(2));
   const args = parsed.operands.concat(parsed.unknown);
   this.args = args.slice();
 
@@ -822,57 +821,6 @@ Command.prototype.executeSubCommand = function(argv, args, executableFile) {
   this.runningCommand = proc;
 };
 
-/**
- * Normalize `args`, splitting joined short flags. For example
- * the arg "-abc" is equivalent to "-a -b -c".
- * This also normalizes equal sign and splits "--abc=def" into "--abc def".
- *
- * @param {Array} args
- * @return {Array}
- * @api private
- */
-
-Command.prototype.normalize = function(args) {
-  var ret = [],
-    arg,
-    lastOpt,
-    index,
-    short,
-    opt;
-
-  for (var i = 0, len = args.length; i < len; ++i) {
-    arg = args[i];
-    if (i > 0) {
-      lastOpt = this.optionFor(args[i - 1]);
-    }
-
-    if (arg === '--') {
-      // Honor option terminator
-      ret = ret.concat(args.slice(i));
-      break;
-    } else if (lastOpt && lastOpt.required) {
-      ret.push(arg);
-    } else if (arg.length > 2 && arg[0] === '-' && arg[1] !== '-') {
-      short = arg.slice(0, 2);
-      opt = this.optionFor(short);
-      if (opt && (opt.required || opt.optional)) {
-        ret.push(short);
-        ret.push(arg.slice(2));
-      } else {
-        arg.slice(1).split('').forEach(function(c) {
-          ret.push('-' + c);
-        });
-      }
-    } else if (/^--/.test(arg) && ~(index = arg.indexOf('='))) {
-      ret.push(arg.slice(0, index), arg.slice(index + 1));
-    } else {
-      ret.push(arg);
-    }
-  }
-
-  return ret;
-};
-
 /**
  * Parse command `args`.
  *
@@ -960,56 +908,78 @@ Command.prototype._checkForMissingMandatoryOptions = function() {
 Command.prototype.parseOptions = function(argv) {
   const operands = []; // operands, not options or values
   const unknown = []; // first unknown option and remaining unknown args
-  let literal = false;
   let dest = operands;
+  const args = argv.slice();
+
+  function maybeOption(arg) {
+    return arg.length > 1 && arg[0] === '-';
+  }
 
   // parse options
-  for (var i = 0; i < argv.length; ++i) {
-    const arg = argv[i];
+  while (args.length) {
+    const arg = args.shift();
 
-    // literal args after --
-    if (literal) {
-      dest.push(arg);
-      continue;
+    // literal
+    if (arg === '--') {
+      if (dest === unknown) dest.push(arg);
+      dest.push(...args);
+      break;
     }
 
-    if (arg === '--') {
-      literal = true;
-      if (dest === unknown) dest.push('--');
-      continue;
+    if (maybeOption(arg)) {
+      const option = this.optionFor(arg);
+      // recognised option, call listener to assign value with possible custom processing
+      if (option) {
+        if (option.required) {
+          const value = args.shift();
+          if (value === undefined) this.optionMissingArgument(option);
+          this.emit(`option:${option.name()}`, value);
+        } else if (option.optional) {
+          let value = null;
+          // historical behaviour is optional value is following arg unless an option
+          if (args.length > 0 && !maybeOption(args[0])) {
+            value = args.shift();
+          }
+          this.emit(`option:${option.name()}`, value);
+        } else { // boolean flag
+          this.emit(`option:${option.name()}`);
+        }
+        continue;
+      }
     }
 
-    // find matching Option
-    const option = this.optionFor(arg);
-
-    // recognised option, call listener to assign value with possible custom processing
-    if (option) {
-      if (option.required) {
-        const value = argv[++i];
-        if (value === undefined) this.optionMissingArgument(option);
-        this.emit('option:' + option.name(), value);
-      } else if (option.optional) {
-        let value = argv[i + 1];
-        // do not use a following option as a value
-        if (value === undefined || (value[0] === '-' && value !== '-')) {
-          value = null;
+    // Look for combo options following single dash, eat first one if known.
+    if (arg.length > 2 && arg[0] === '-' && arg[1] !== '-') {
+      const option = this.optionFor(`-${arg[1]}`);
+      if (option) {
+        if (option.required || option.optional) {
+          // option with value following in same argument
+          this.emit(`option:${option.name()}`, arg.slice(2));
         } else {
-          ++i;
+          // boolean option, emit and put back remainder of arg for further processing
+          this.emit(`option:${option.name()}`);
+          args.unshift(`-${arg.slice(2)}`);
         }
-        this.emit('option:' + option.name(), value);
-      // flag
-      } else {
-        this.emit('option:' + option.name());
+        continue;
+      }
+    }
+
+    // Look for known long flag with value, like --foo=bar
+    if (/^--[^=]+=/.test(arg)) {
+      const index = arg.indexOf('=');
+      const option = this.optionFor(arg.slice(0, index));
+      if (option && (option.required || option.optional)) {
+        this.emit(`option:${option.name()}`, arg.slice(index + 1));
+        continue;
       }
-      continue;
     }
 
-    // looks like an option, unknowns from here
+    // looks like an option but unknown, unknowns from here
     if (arg.length > 1 && arg[0] === '-') {
       dest = unknown;
     }
 
-    // arg
+    // add arg
     dest.push(arg);
   }
 

tests/args.literal.test.js

@@ -1,9 +1,9 @@
 const commander = require('../');
 
-// Guideline 10:
-// The first -- argument that is not an option-argument should be accepted as a delimiter indicating the end of options. Any following arguments should be treated as operands, even if they begin with the '-' character.
+// Utility Conventions: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02
 //
-// http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02
+// 12.2 Utility Syntax Guidelines, Guideline 10:
+// The first -- argument that is not an option-argument should be accepted as a delimiter indicating the end of options. Any following arguments should be treated as operands, even if they begin with the '-' character.
 
 test('when arguments includes -- then stop processing options', () => {
   const program = new commander.Command();

tests/command.parseOptions.test.js

@@ -7,17 +7,6 @@ const path = require('path');
 
 // Tests created from reported bugs
 describe('regression tests', () => {
-  // https://github.com/tj/commander.js/issues/1039
-  // https://github.com/tj/commander.js/issues/561
-  test('when unknown option and multiple arguments then unknown option detected', () => {
-    const program = new commander.Command();
-    program
-      .exitOverride();
-    expect(() => {
-      program.parse(['node', 'text', '--bug', '0', '1', '2', '3']);
-    }).toThrow();
-  });
-
   // https://github.com/tj/commander.js/issues/1032
   function createProgram1032() {
     const program = new commander.Command();
@@ -196,3 +185,106 @@ describe('parse and program.args', () => {
     expect(program.args).toEqual(['sub', '--sub-flag', 'arg']);
   });
 });
+
+// Utility Conventions: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_01
+//
+// 12.1 Utility Argument Syntax
+// 2. Option-arguments are shown separated from their options by <blank> characters, except when the option-argument is
+// enclosed in the '[' and ']' notation to indicate that it is optional. This reflects the situation in which an
+// optional option-argument (if present) is included within the same argument string as the option; for a mandatory option-argument,
+// it is the next argument. The Utility Syntax Guidelines in Utility Syntax Guidelines require that the option be a separate argument
+// from its option-argument and that option-arguments not be optional, but there are some exceptions in POSIX.1-2017 to ensure
+// continued operation of historical applications:
+//
+// a. If the SYNOPSIS of a standard utility shows an option with a mandatory option-argument (as with [ -c option_argument] in the example),
+//    a conforming application shall use separate arguments for that option and its option-argument. However, a conforming implementation shall
+//    also permit applications to specify the option and option-argument in the same argument string without intervening <blank> characters.
+//
+// b. If the SYNOPSIS shows an optional option-argument (as with [ -f[ option_argument]] in the example), a conforming application
+//    shall place any option-argument for that option directly adjacent to the option in the same argument string, without intervening
+//    <blank> characters. If the utility receives an argument containing only the option, it shall behave as specified in its description
+//    for an omitted option-argument; it shall not treat the next argument (if any) as the option-argument for that option.
+//
+// 12.2 Utility Syntax Guidelines, Guideline 5:
+// 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.
+
+// Commander conformance:
+// - allows separate argument for required option-argument
+// - allows value in same argument for short flag with required option-argument
+// - non-conforming: allows separate argument for optional option-argument if does not start with '-'
+// - allows value in same argument for short flag with optional option-argument
+// - allows short flags as per 12.2
+//
+// The focus in this file is testing the behaviours with known vs unknown options.
+// See options.values.test.js for more general testing of known options.
+
+describe('Utility Conventions', () => {
+  function createProgram() {
+    const program = new commander.Command();
+    program
+      .option('-a,--aaa')
+      .option('-b,--bbb')
+      .option('-c,--ccc <value>')
+      .option('-d,--ddd [value]');
+    program
+      .action(() => { });
+    return program;
+  };
+
+  test('when program has combo known boolean short flags then arg removed', () => {
+    const program = createProgram();
+    const result = program.parseOptions(['-ab']);
+    expect(result).toEqual({ operands: [], unknown: [] });
+    expect(program.opts()).toEqual({ aaa: true, bbb: true });
+  });
+
+  test('when program has combo unknown short flags then arg preserved', () => {
+    const program = createProgram();
+    const result = program.parseOptions(['-pq']);
+    expect(result).toEqual({ operands: [], unknown: ['-pq'] });
+    expect(program.opts()).toEqual({ });
+  });
+
+  test('when program has combo known short option and required value then arg removed', () => {
+    const program = createProgram();
+    const result = program.parseOptions(['-cvalue']);
+    expect(result).toEqual({ operands: [], unknown: [] });
+    expect(program.opts()).toEqual({ ccc: 'value' });
+  });
+
+  test('when program has combo known short option and optional value then arg removed', () => {
+    const program = createProgram();
+    const result = program.parseOptions(['-dvalue']);
+    expect(result).toEqual({ operands: [], unknown: [] });
+    expect(program.opts()).toEqual({ ddd: 'value' });
+  });
+
+  test('when program has known combo short boolean flags and required value then arg removed', () => {
+    const program = createProgram();
+    const result = program.parseOptions(['-abcvalue']);
+    expect(result).toEqual({ operands: [], unknown: [] });
+    expect(program.opts()).toEqual({ aaa: true, bbb: true, ccc: 'value' });
+  });
+
+  test('when program has known combo short boolean flags and optional value then arg removed', () => {
+    const program = createProgram();
+    const result = program.parseOptions(['-abdvalue']);
+    expect(result).toEqual({ operands: [], unknown: [] });
+    expect(program.opts()).toEqual({ aaa: true, bbb: true, ddd: 'value' });
+  });
+
+  test('when program has known long flag=value then arg removed', () => {
+    const program = createProgram();
+    const result = program.parseOptions(['--ccc=value']);
+    expect(result).toEqual({ operands: [], unknown: [] });
+    expect(program.opts()).toEqual({ ccc: 'value' });
+  });
+
+  test('when program has unknown long flag=value then arg preserved', () => {
+    const program = createProgram();
+    const result = program.parseOptions(['--rrr=value']);
+    expect(result).toEqual({ operands: [], unknown: ['--rrr=value'] });
+    expect(program.opts()).toEqual({ });
+  });
+});

tests/openIssues.test.js.skip

@@ -1,4 +1,16 @@
 const commander = require('../');
 
 describe('open issues', () => {
+  // https://github.com/tj/commander.js/issues/561
+  test('#561: when specify argument and unknown option with no action handler then error', () => {
+    const program = new commander.Command();
+    program
+      .exitOverride()
+      .option('-x, --x-flag', 'A flag')
+      .arguments('<file>');
+
+    expect(() => {
+      program.parse(['node', 'test', '1', '--NONSENSE', '2', '3']);
+    }).toThrow();
+  });
 });

tests/options.values.test.js

@@ -1,6 +1,7 @@
 const commander = require('../');
 
 // Test the ways values can be specified for options.
+// See also references on "Utility Conventions" in command.parseOptions.test.js
 
 // options with required values can eat values starting with a dash, including just dash sometimes used as alias for stdin
 //