Warn about obscured help option

lib/command.js

@@ -7,7 +7,7 @@ const process = require('process');
 const { Argument, humanReadableArgName } = require('./argument.js');
 const { CommanderError } = require('./error.js');
 const { Help } = require('./help.js');
-const { Option, splitOptionFlags, DualOptions } = require('./option.js');
+const { Option, DualOptions } = require('./option.js');
 const { suggestSimilar } = require('./suggestSimilar');
 
 // @ts-check
@@ -67,16 +67,13 @@ class Command extends EventEmitter {
     };
 
     this._hidden = false;
-    this._hasHelpOption = true;
-    this._helpFlags = '-h, --help';
-    this._helpDescription = 'display help for command';
-    this._helpShortFlag = '-h';
-    this._helpLongFlag = '--help';
     this._addImplicitHelpCommand = undefined; // Deliberately undefined, not decided whether true or false
+
+    this._helpConfiguration = {};
     this._helpCommandName = 'help';
     this._helpCommandnameAndArgs = 'help [command]';
     this._helpCommandDescription = 'display help for command';
-    this._helpConfiguration = {};
+    this.helpOption('-h, --help', 'display help for command');
   }
 
   /**
@@ -89,15 +86,6 @@ class Command extends EventEmitter {
    */
   copyInheritedSettings(sourceCommand) {
     this._outputConfiguration = sourceCommand._outputConfiguration;
-    this._hasHelpOption = sourceCommand._hasHelpOption;
-    this._helpFlags = sourceCommand._helpFlags;
-    this._helpDescription = sourceCommand._helpDescription;
-    this._helpShortFlag = sourceCommand._helpShortFlag;
-    this._helpLongFlag = sourceCommand._helpLongFlag;
-    this._helpCommandName = sourceCommand._helpCommandName;
-    this._helpCommandnameAndArgs = sourceCommand._helpCommandnameAndArgs;
-    this._helpCommandDescription = sourceCommand._helpCommandDescription;
-    this._helpConfiguration = sourceCommand._helpConfiguration;
     this._exitCallback = sourceCommand._exitCallback;
     this._storeOptionsAsProperties = sourceCommand._storeOptionsAsProperties;
     this._combineFlagAndOptionalValue = sourceCommand._combineFlagAndOptionalValue;
@@ -106,6 +94,16 @@ class Command extends EventEmitter {
     this._showHelpAfterError = sourceCommand._showHelpAfterError;
     this._showSuggestionAfterError = sourceCommand._showSuggestionAfterError;
 
+    this._helpConfiguration = sourceCommand._helpConfiguration;
+    this._helpCommandName = sourceCommand._helpCommandName;
+    this._helpCommandnameAndArgs = sourceCommand._helpCommandnameAndArgs;
+    this._helpCommandDescription = sourceCommand._helpCommandDescription;
+    if (sourceCommand._hasHelpOption) {
+      this.helpOption(sourceCommand._helpFlags, sourceCommand._helpDescription);
+    } else {
+      this._hasHelpOption = false;
+    }
+
     return this;
   }
 
@@ -500,13 +498,33 @@ Expecting one of '${allowedValues.join("', '")}'`);
     return new Option(flags, description);
   }
 
+  /**
+   * Check for option flag conflicts.
+   * Register option if no conflicts found.
+   * Throw otherwise.
+   *
+   * @param {Option} option
+   * @api private
+   */
+
+  _registerOption(option) {
+    this.options.push(option);
+    this._checkForObscuredHelpOption((matchingOptionFlags) => (
+      `Help option '${this._helpOption.flags}' is obscured after adding option '${option.flags}'${matchingOptionFlags.length > 1
+        ? `
+- conflicts with options '${matchingOptionFlags.join("' and '")}'`
+        : ''}`));
+  }
+
   /**
    * 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 +539,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.
@@ -1101,7 +1116,9 @@ Expecting one of '${allowedValues.join("', '")}'`);
     }
 
     // Fallback to parsing the help flag to invoke the help.
-    return this._dispatchSubcommand(subcommandName, [], [this._helpLongFlag]);
+    return this._dispatchSubcommand(subcommandName, [], [
+      this._helpOption.long || this._helpOption.short
+    ]);
   }
 
   /**
@@ -1820,7 +1837,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);
@@ -2034,7 +2051,9 @@ Expecting one of '${allowedValues.join("', '")}'`);
     }
     context.write(helpInformation);
 
-    this.emit(this._helpLongFlag); // deprecated
+    if (this._helpOption.long) {
+      this.emit(this._helpOption.long); // deprecated
+    }
     this.emit('afterHelp', context);
     getCommandAndParents(this).forEach(command => command.emit('afterAllHelp', context));
   }
@@ -2050,20 +2069,45 @@ Expecting one of '${allowedValues.join("', '")}'`);
    */
 
   helpOption(flags, description) {
-    if (typeof flags === 'boolean') {
-      this._hasHelpOption = flags;
+    if (flags !== undefined && typeof flags !== 'string') {
+      this._hasHelpOption = !!flags;
       return this;
     }
-    this._helpFlags = flags || this._helpFlags;
-    this._helpDescription = description || this._helpDescription;
+    this._hasHelpOption = true;
+    this._helpFlags = flags = flags || this._helpFlags;
+    this._helpDescription = description = description || this._helpDescription;
 
-    const helpFlags = splitOptionFlags(this._helpFlags);
-    this._helpShortFlag = helpFlags.shortFlag;
-    this._helpLongFlag = helpFlags.longFlag;
+    this._helpOption = this.createOption(flags, description);
+    this._checkForObscuredHelpOption((matchingOptionFlags) => (
+      `Newly added help option '${this._helpOption.flags}' is obscured
+- conflicts with ${matchingOptionFlags.length > 1 ? 'options' : 'option'} '${matchingOptionFlags.join("' and '")}'`));
 
     return this;
   }
 
+  /**
+   * @api private
+   */
+
+  _checkForObscuredHelpOption(makeMessage) {
+    if (this._hasHelpOption) {
+      const shortMatchingOption = this._helpOption.short &&
+        this._findOption(this._helpOption.short);
+      if (shortMatchingOption || !this._helpOption.short) {
+        const longMatchingOption = this._helpOption.long &&
+          this._findOption(this._helpOption.long);
+        if (longMatchingOption || !this._helpOption.long) {
+          const matchingOptionFlags = (
+            shortMatchingOption === longMatchingOption
+              ? [shortMatchingOption]
+              : [shortMatchingOption, longMatchingOption]
+          ).filter(option => option).map(option => option.flags);
+          console.warn(makeMessage(matchingOptionFlags));
+        }
+      }
+    }
+  }
+
   /**
    * Output help information and exit.
    *
@@ -2124,7 +2168,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
  */
 
 function outputHelpIfRequested(cmd, args) {
-  const helpOption = cmd._hasHelpOption && args.find(arg => arg === cmd._helpLongFlag || arg === cmd._helpShortFlag);
+  const helpOption = cmd._hasHelpOption && args.find(arg => arg === cmd._helpOption.long || arg === cmd._helpOption.short);
   if (helpOption) {
     cmd.outputHelp();
     // (Do not have all displayed text available so only passing placeholder.)

lib/help.js

@@ -71,14 +71,14 @@ class Help {
   visibleOptions(cmd) {
     const visibleOptions = cmd.options.filter((option) => !option.hidden);
     // Implicit help
-    const showShortHelpFlag = cmd._hasHelpOption && cmd._helpShortFlag && !cmd._findOption(cmd._helpShortFlag);
-    const showLongHelpFlag = cmd._hasHelpOption && !cmd._findOption(cmd._helpLongFlag);
+    const showShortHelpFlag = cmd._hasHelpOption && cmd._helpOption.short && !cmd._findOption(cmd._helpOption.short);
+    const showLongHelpFlag = cmd._hasHelpOption && !cmd._findOption(cmd._helpOption.long);
     if (showShortHelpFlag || showLongHelpFlag) {
       let helpOption;
       if (!showShortHelpFlag) {
-        helpOption = cmd.createOption(cmd._helpLongFlag, cmd._helpDescription);
+        helpOption = cmd.createOption(cmd._helpOption.long, cmd._helpDescription);
       } else if (!showLongHelpFlag) {
-        helpOption = cmd.createOption(cmd._helpShortFlag, cmd._helpDescription);
+        helpOption = cmd.createOption(cmd._helpOption.short, cmd._helpDescription);
       } else {
         helpOption = cmd.createOption(cmd._helpFlags, cmd._helpDescription);
       }

lib/option.js

@@ -220,13 +220,13 @@ class Option {
   /**
    * Check if `arg` matches the short or long flag.
    *
-   * @param {string} arg
+   * @param {string | undefined} arg
    * @return {boolean}
    * @api private
    */
 
   is(arg) {
-    return this.short === arg || this.long === arg;
+    return arg !== undefined && (this.short === arg || this.long === arg);
   }
 
   /**

tests/command.addHelpText.test.js

@@ -181,7 +181,7 @@ describe('context checks with full parse', () => {
   });
 
   test('when help requested then context.error is false', () => {
-    let context = {};
+    let context;
     const program = new commander.Command();
     program
       .exitOverride()
@@ -193,7 +193,7 @@ describe('context checks with full parse', () => {
   });
 
   test('when help for error then context.error is true', () => {
-    let context = {};
+    let context;
     const program = new commander.Command();
     program
       .exitOverride()
@@ -206,7 +206,7 @@ describe('context checks with full parse', () => {
   });
 
   test('when help on program then context.command is program', () => {
-    let context = {};
+    let context;
     const program = new commander.Command();
     program
       .exitOverride()
@@ -218,7 +218,7 @@ describe('context checks with full parse', () => {
   });
 
   test('when help on subcommand and "before" subcommand then context.command is subcommand', () => {
-    let context = {};
+    let context;
     const program = new commander.Command();
     program
       .exitOverride();
@@ -231,7 +231,7 @@ describe('context checks with full parse', () => {
   });
 
   test('when help on subcommand and "beforeAll" on program then context.command is subcommand', () => {
-    let context = {};
+    let context;
     const program = new commander.Command();
     program
       .exitOverride()

tests/command.copySettings.test.js

@@ -44,10 +44,11 @@ describe('copyInheritedSettings property tests', () => {
 
     source.helpOption('-Z, --zz', 'ddd');
     cmd.copyInheritedSettings(source);
+    expect(cmd._hasHelpOption).toBeTruthy();
     expect(cmd._helpFlags).toBe('-Z, --zz');
     expect(cmd._helpDescription).toBe('ddd');
-    expect(cmd._helpShortFlag).toBe('-Z');
-    expect(cmd._helpLongFlag).toBe('--zz');
+    expect(cmd._helpOption.short).toBe('-Z');
+    expect(cmd._helpOption.long).toBe('--zz');
   });
 
   test('when copyInheritedSettings then copies addHelpCommand(name, description)', () => {