Add .strictOptionalOptionArguments() for POSIX-style handling of options with optional option-arguments

lib/command.js

@@ -46,6 +46,7 @@ class Command extends EventEmitter {
     this._defaultCommandName = null;
     this._exitCallback = null;
     this._aliases = [];
+    this._strictOptionalOptionArguments = false;
     this._combineFlagAndOptionalValue = true;
     this._description = '';
     this._summary = '';
@@ -100,6 +101,7 @@ class Command extends EventEmitter {
     this._helpConfiguration = sourceCommand._helpConfiguration;
     this._exitCallback = sourceCommand._exitCallback;
     this._storeOptionsAsProperties = sourceCommand._storeOptionsAsProperties;
+    this._strictOptionalOptionArguments = sourceCommand._strictOptionalOptionArguments;
     this._combineFlagAndOptionalValue = sourceCommand._combineFlagAndOptionalValue;
     this._allowExcessArguments = sourceCommand._allowExcessArguments;
     this._enablePositionalOptions = sourceCommand._enablePositionalOptions;
@@ -500,13 +502,28 @@ Expecting one of '${allowedValues.join("', '")}'`);
     return new Option(flags, description);
   }
 
+  /**
+   * Register option if possible with current settings.
+   * Throw otherwise.
+   *
+   * @param {Option} option
+   * @api private
+   */
+
+  _registerOption(option) {
+    this._checkForBrokenOptionalOptionArguments(option);
+    this.options.push(option);
+  }
+
   /**
    * Add an option.
    *
    * @param {Option} option
    * @return {Command} `this` command for chaining
    */
   addOption(option) {
+    this._registerOption(option);
+
     const oname = option.name();
     const name = option.attributeName();
 
@@ -521,9 +538,6 @@ Expecting one of '${allowedValues.join("', '")}'`);
       this.setOptionValueWithSource(name, option.defaultValue, 'default');
     }
 
-    // register the option
-    this.options.push(option);
-
     // handler for cli and env supplied values
     const handleOptionValue = (val, invalidValueMessage, valueSource) => {
       // val is null for optional option used without an optional-argument.
@@ -675,6 +689,55 @@ Expecting one of '${allowedValues.join("', '")}'`);
     return this._optionEx({ mandatory: true }, flags, description, fn, defaultValue);
   }
 
+  /**
+   * Check if the passed option or any registered option is unusable due to the current `strictOptionalOptionArguments` setting.
+   * Throw if the check result is positive.
+   *
+   * An option is deemed unusable when
+   * - `strictOptionalOptionArguments` is on,
+   * - and the option has an optional option-argument,
+   * - and one of the following holds true:
+   *   - `combineFlagAndOptionalValue` is off,
+   *   - or the option is variadic.
+   *
+   * @param {Option} [option]
+   * @api private
+   */
+
+  _checkForBrokenOptionalOptionArguments(option) {
+    if (this._strictOptionalOptionArguments) {
+      const options = option ? [option] : this.options;
+      if (!this._combineFlagAndOptionalValue) {
+        options.forEach((option) => {
+          if (option.optional && !option.long) {
+            throw new Error(`Option '${option.flags}' is incompatible with strictOptionalOptionArguments${this._name ? ` enabled for '${this._name}'` : ''} when combineFlagAndOptionalValue is off
+- must have a long flag`);
+          }
+        });
+      }
+      options.forEach((option) => {
+        if (option.optional && option.variadic) {
+          throw new Error(`Variadic option '${option.flags}' is incompatible with strictOptionalOptionArguments${this._name ? ` enabled for '${this._name}'` : ''}`);
+        }
+      });
+    }
+  }
+
+  /**
+   * When set to `true`, handle options with optional option-arguments as prescribed by POSIX,
+   * i.e. only allow providing values for such option-arguments by combining them with a flag
+   * (e.g. `-cmozzarella`, `--cheese=mozzarella`).
+   *
+   * @param {Boolean} [strict=true]
+   * @return {Command} `this` command for chaining
+   */
+
+  strictOptionalOptionArguments(strict = true) {
+    this._strictOptionalOptionArguments = !!strict;
+    this._checkForBrokenOptionalOptionArguments();
+    return this;
+  }
+
   /**
    * Alter parsing of short flags with optional values.
    *
@@ -687,6 +750,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
    */
   combineFlagAndOptionalValue(combine = true) {
     this._combineFlagAndOptionalValue = !!combine;
+    this._checkForBrokenOptionalOptionArguments();
     return this;
   }
 
@@ -1439,12 +1503,9 @@ Expecting one of '${allowedValues.join("', '")}'`);
     let dest = operands;
     const args = argv.slice();
 
-    function maybeOption(arg) {
-      return arg.length > 1 && arg[0] === '-';
-    }
-
     // parse options
     let activeVariadicOption = null;
+    let onlyKnownOptionsSoFar = true;
     while (args.length) {
       const arg = args.shift();
 
@@ -1455,13 +1516,15 @@ Expecting one of '${allowedValues.join("', '")}'`);
         break;
       }
 
-      if (activeVariadicOption && !maybeOption(arg)) {
+      const isArgFlag = isFlag(arg);
+
+      if (activeVariadicOption && !isArgFlag) {
         this.emit(`option:${activeVariadicOption.name()}`, arg);
         continue;
       }
       activeVariadicOption = null;
 
-      if (maybeOption(arg)) {
+      if (isArgFlag) {
         const option = this._findOption(arg);
         // recognised option, call listener to assign value with possible custom processing
         if (option) {
@@ -1472,7 +1535,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
           } 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])) {
+            if (!(this._strictOptionalOptionArguments) && args.length > 0 && !isFlag(args[0])) {
               value = args.shift();
             }
             this.emit(`option:${option.name()}`, value);
@@ -1482,63 +1545,71 @@ Expecting one of '${allowedValues.join("', '")}'`);
           activeVariadicOption = option.variadic ? option : null;
           continue;
         }
-      }
 
-      // Look for combo options following single dash, eat first one if known.
-      if (arg.length > 2 && arg[0] === '-' && arg[1] !== '-') {
-        const option = this._findOption(`-${arg[1]}`);
-        if (option) {
-          if (option.required || (option.optional && this._combineFlagAndOptionalValue)) {
-            // option with value following in same argument
-            this.emit(`option:${option.name()}`, arg.slice(2));
-          } else {
-            // boolean option, emit and put back remainder of arg for further processing
-            this.emit(`option:${option.name()}`);
-            args.unshift(`-${arg.slice(2)}`);
+        // Look for combo options following single dash, eat first one if known.
+        if (arg.length > 2 && arg[1] !== '-') {
+          const option = this._findOption(`-${arg[1]}`);
+          if (option) {
+            if (option.required || (option.optional && this._combineFlagAndOptionalValue)) {
+              // option with value following in same argument
+              this.emit(`option:${option.name()}`, arg.slice(2));
+            } else {
+              // boolean option, emit and put back remainder of arg for further processing
+              this.emit(`option:${option.name()}`);
+              args.unshift(`-${arg.slice(2)}`);
+            }
+            continue;
           }
-          continue;
         }
-      }
 
-      // Look for known long flag with value, like --foo=bar
-      if (/^--[^=]+=/.test(arg)) {
-        const index = arg.indexOf('=');
-        const option = this._findOption(arg.slice(0, index));
-        if (option && (option.required || option.optional)) {
-          this.emit(`option:${option.name()}`, arg.slice(index + 1));
-          continue;
+        // Look for known long flag with value, like --foo=bar
+        if (/^--[^=]+=/.test(arg)) {
+          const index = arg.indexOf('=');
+          const option = this._findOption(arg.slice(0, index));
+          if (option && (option.required || option.optional)) {
+            this.emit(`option:${option.name()}`, arg.slice(index + 1));
+            continue;
+          }
         }
       }
 
-      // Not a recognised option by this command.
-      // Might be a command-argument, or subcommand option, or unknown option, or help command or option.
+      // Not a recognised option by this command. Might be
+      // - a subcommand,
+      // - the help option (currently not handled by this routine),
+      // - an option unknown to this command,
+      // - or a command-argument.
+
+      if (onlyKnownOptionsSoFar) {
+        const stopAtSubcommand = (
+          this._enablePositionalOptions || this._passThroughOptions
+        );
+        if (stopAtSubcommand) {
+          if (!isArgFlag && this._findCommand(arg)) {
+            operands.push(arg);
+            unknown.push(...args);
+            break;
+          } else if (!isArgFlag &&
+            arg === this._helpCommandName &&
+            this._hasImplicitHelpCommand()
+          ) {
+            operands.push(arg, ...args);
+            break;
+          } else if (this._defaultCommandName) {
+            unknown.push(arg, ...args);
+            break;
+          }
+        }
+      }
+      onlyKnownOptionsSoFar = false;
 
       // An unknown option means further arguments also classified as unknown so can be reprocessed by subcommands.
-      if (maybeOption(arg)) {
+      if (isArgFlag) {
         dest = unknown;
       }
 
-      // If using positionalOptions, stop processing our options at subcommand.
-      if ((this._enablePositionalOptions || this._passThroughOptions) && operands.length === 0 && unknown.length === 0) {
-        if (this._findCommand(arg)) {
-          operands.push(arg);
-          if (args.length > 0) unknown.push(...args);
-          break;
-        } else if (arg === this._helpCommandName && this._hasImplicitHelpCommand()) {
-          operands.push(arg);
-          if (args.length > 0) operands.push(...args);
-          break;
-        } else if (this._defaultCommandName) {
-          unknown.push(arg);
-          if (args.length > 0) unknown.push(...args);
-          break;
-        }
-      }
-
       // If using passThroughOptions, stop processing options at first command-argument.
       if (this._passThroughOptions) {
-        dest.push(arg);
-        if (args.length > 0) dest.push(...args);
+        dest.push(arg, ...args);
         break;
       }
 
@@ -1820,7 +1891,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
     description = description || 'output the version number';
     const versionOption = this.createOption(flags, description);
     this._versionOptionName = versionOption.attributeName();
-    this.options.push(versionOption);
+    this._registerOption(versionOption);
     this.on('option:' + versionOption.name(), () => {
       this._outputConfiguration.writeOut(`${str}\n`);
       this._exit(0, 'commander.version', str);
@@ -2193,4 +2264,14 @@ function getCommandAndParents(startCommand) {
   return result;
 }
 
+/**
+ * @param {string} arg
+ * @returns {boolean}
+ * @api private
+ */
+
+function isFlag(arg) {
+  return arg.length > 1 && arg[0] === '-';
+}
+
 exports.Command = Command;

lib/help.js

@@ -153,11 +153,14 @@ class Help {
    * Get the option term to show in the list of options.
    *
    * @param {Option} option
+   * @param {Command} [cmd]
    * @returns {string}
    */
 
-  optionTerm(option) {
-    return option.flags;
+  optionTerm(option, cmd) {
+    return cmd?._strictOptionalOptionArguments && option.optional
+      ? option.strictOptionalHelpTerm
+      : option.flags;
   }
 
   /**
@@ -195,7 +198,7 @@ class Help {
 
   longestOptionTermLength(cmd, helper) {
     return helper.visibleOptions(cmd).reduce((max, option) => {
-      return Math.max(max, helper.optionTerm(option).length);
+      return Math.max(max, helper.optionTerm(option, cmd).length);
     }, 0);
   }
 
@@ -209,7 +212,7 @@ class Help {
 
   longestGlobalOptionTermLength(cmd, helper) {
     return helper.visibleGlobalOptions(cmd).reduce((max, option) => {
-      return Math.max(max, helper.optionTerm(option).length);
+      return Math.max(max, helper.optionTerm(option, cmd).length);
     }, 0);
   }
 
@@ -380,15 +383,15 @@ class Help {
 
     // Options
     const optionList = helper.visibleOptions(cmd).map((option) => {
-      return formatItem(helper.optionTerm(option), helper.optionDescription(option));
+      return formatItem(helper.optionTerm(option, cmd), helper.optionDescription(option));
     });
     if (optionList.length > 0) {
       output = output.concat(['Options:', formatList(optionList), '']);
     }
 
     if (this.showGlobalOptions) {
       const globalOptionList = helper.visibleGlobalOptions(cmd).map((option) => {
-        return formatItem(helper.optionTerm(option), helper.optionDescription(option));
+        return formatItem(helper.optionTerm(option, cmd), helper.optionDescription(option));
       });
       if (globalOptionList.length > 0) {
         output = output.concat(['Global Options:', formatList(globalOptionList), '']);

lib/option.js

@@ -22,6 +22,23 @@ class Option {
     const optionFlags = splitOptionFlags(flags);
     this.short = optionFlags.shortFlag;
     this.long = optionFlags.longFlag;
+    this.strictOptionalHelpTerm = null;
+    if (this.optional) {
+      const indexStart = flags.indexOf('[');
+      const indexEnd = indexStart + flags.slice(indexStart).indexOf(']') + 1;
+      const argument = flags.slice(indexStart, indexEnd);
+      this.strictOptionalHelpTerm = (
+        flags.slice(0, indexStart) + flags.slice(indexEnd)
+      );
+      if (this.short) {
+        this.strictOptionalHelpTerm = this.strictOptionalHelpTerm
+          .replace(this.short, `${this.short}${argument}`);
+      }
+      if (this.long) {
+        this.strictOptionalHelpTerm = this.strictOptionalHelpTerm
+          .replace(this.long, `${this.long}=${argument}`);
+      }
+    }
     this.negate = false;
     if (this.long) {
       this.negate = this.long.startsWith('--no-');