Rework option defaults and add preset (#1652)

* Add defaultOption, but not implemented

* Start refactoring addOption to clarify logic

* Update comments

* Implement separate default and preset for options

* Changed behaviour of boolean default, fix and extend test

* Update unit tests with new default/preset behaviour

* Add tests for default with all option types

* Add tests that default does get overwritten

* Fix JSDoc for new routines

* Add tests for preset

* Add typings

* Update README

* Add test of preset with negated

* Update comments

* Add tests of options being used twice

* Modify documentation for preset

* Add prefix example usage in README

* Add preset to help

* Update code example

* Use same format for preset as for default, add test

* Be selective about default and preset shown in help

Readme.md

@@ -146,7 +146,7 @@ pizza details:
 
 ### Default option value
 
-You can specify a default value for an option which takes a value.
+You can specify a default value for an option.
 
 Example file: [options-defaults.js](./examples/options-defaults.js)
 
@@ -172,7 +172,7 @@ You can define a boolean option long name with a leading `no-` to set the option
 Defined alone this also makes the option true by default.
 
 If you define `--foo` first, adding `--no-foo` does not change the default value from what it would
-otherwise be. You can specify a default boolean value for a boolean option and it can be overridden on command line.
+otherwise be.
 
 Example file: [options-negatable.js](./examples/options-negatable.js)
 
@@ -312,7 +312,8 @@ program
   .addOption(new Option('-s, --secret').hideHelp())
   .addOption(new Option('-t, --timeout <delay>', 'timeout in seconds').default(60, 'one minute'))
   .addOption(new Option('-d, --drink <size>', 'drink size').choices(['small', 'medium', 'large']))
-  .addOption(new Option('-p, --port <number>', 'port number').env('PORT'));
+  .addOption(new Option('-p, --port <number>', 'port number').env('PORT'))
+  .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat));
 ```
 
 ```bash
@@ -323,13 +324,14 @@ Options:
   -t, --timeout <delay>  timeout in seconds (default: one minute)
   -d, --drink <size>     drink cup size (choices: "small", "medium", "large")
   -p, --port <number>    port number (env: PORT)
+  --donate [amount]      optional donation in dollars (preset: 20)
   -h, --help             display help for command
 
 $ extra --drink huge
 error: option '-d, --drink <size>' argument 'huge' is invalid. Allowed choices are small, medium, large.
 
-$ PORT=80 extra 
-Options:  { timeout: 60, port: '80' }
+$ PORT=80 extra --donate
+Options:  { timeout: 60, donate: 20, port: '80' }
 ```
 
 ### Custom option processing

examples/options-extra.js

@@ -11,7 +11,8 @@ program
   .addOption(new Option('-s, --secret').hideHelp())
   .addOption(new Option('-t, --timeout <delay>', 'timeout in seconds').default(60, 'one minute'))
   .addOption(new Option('-d, --drink <size>', 'drink cup size').choices(['small', 'medium', 'large']))
-  .addOption(new Option('-p, --port <number>', 'port number').env('PORT'));
+  .addOption(new Option('-p, --port <number>', 'port number').env('PORT'))
+  .addOption(new Option('--donate [amount]', 'optional donation in dollars').preset('20').argParser(parseFloat));
 
 program.parse();
 
@@ -21,3 +22,5 @@ console.log('Options: ', program.opts());
 //    node options-extra.js --help
 //    node options-extra.js --drink huge
 //    PORT=80 node options-extra.js
+//    node options-extra.js --donate
+//    node options-extra.js --donate 30.50

lib/command.js

@@ -508,33 +508,33 @@ Expecting one of '${allowedValues.join("', '")}'`);
     const oname = option.name();
     const name = option.attributeName();
 
-    let defaultValue = option.defaultValue;
-
-    // preassign default value for --no-*, [optional], <required>, or plain flag if boolean value
-    if (option.negate || option.optional || option.required || typeof defaultValue === 'boolean') {
-      // when --no-foo we make sure default is true, unless a --foo option is already defined
-      if (option.negate) {
-        const positiveLongFlag = option.long.replace(/^--no-/, '--');
-        defaultValue = this._findOption(positiveLongFlag) ? this.getOptionValue(name) : true;
-      }
-      // preassign only if we have a default
-      if (defaultValue !== undefined) {
-        this.setOptionValueWithSource(name, defaultValue, 'default');
+    // store default value
+    if (option.negate) {
+      // --no-foo is special and defaults foo to true, unless a --foo option is already defined
+      const positiveLongFlag = option.long.replace(/^--no-/, '--');
+      if (!this._findOption(positiveLongFlag)) {
+        this.setOptionValueWithSource(name, option.defaultValue === undefined ? true : option.defaultValue, 'default');
       }
+    } else if (option.defaultValue !== undefined) {
+      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) => {
-      // Note: using closure to access lots of lexical scoped variables.
-      const oldValue = this.getOptionValue(name);
+      // val is null for optional option used without an optional-argument.
+      // val is undefined for boolean and negated option.
+      if (val == null && option.presetArg !== undefined) {
+        val = option.presetArg;
+      }
 
       // custom processing
+      const oldValue = this.getOptionValue(name);
       if (val !== null && option.parseArg) {
         try {
-          val = option.parseArg(val, oldValue === undefined ? defaultValue : oldValue);
+          val = option.parseArg(val, oldValue);
         } catch (err) {
           if (err.code === 'commander.invalidArgument') {
             const message = `${invalidValueMessage} ${err.message}`;
@@ -546,18 +546,17 @@ Expecting one of '${allowedValues.join("', '")}'`);
         val = option._concatValue(val, oldValue);
       }
 
-      // unassigned or boolean value
-      if (typeof oldValue === 'boolean' || typeof oldValue === 'undefined') {
-        // if no value, negate false, and we have a default, then use it!
-        if (val == null) {
-          this.setOptionValueWithSource(name, option.negate ? false : defaultValue || true, valueSource);
+      // Fill-in appropriate missing values. Long winded but easy to follow.
+      if (val == null) {
+        if (option.negate) {
+          val = false;
+        } else if (option.isBoolean() || option.optional) {
+          val = true;
         } else {
-          this.setOptionValueWithSource(name, val, valueSource);
+          val = ''; // not normal, parseArg might have failed or be a mock function for testing
         }
-      } else if (val !== null) {
-        // reassign
-        this.setOptionValueWithSource(name, option.negate ? false : val, valueSource);
       }
+      this.setOptionValueWithSource(name, val, valueSource);
     };
 
     this.on('option:' + oname, (val) => {
@@ -813,7 +812,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
 
   /**
    * Get user arguments from implied or explicit arguments.
-   * Side-effects: set _scriptPath if args included application, and use that to set implicit command name.
+   * Side-effects: set _scriptPath if args included script. Used for default program name, and subcommand searches.
    *
    * @api private
    */

lib/help.js

@@ -235,15 +235,24 @@ class Help {
 
   optionDescription(option) {
     const extraInfo = [];
-    // Some of these do not make sense for negated boolean and suppress for backwards compatibility.
 
-    if (option.argChoices && !option.negate) {
+    if (option.argChoices) {
       extraInfo.push(
         // use stringify to match the display of the default value
         `choices: ${option.argChoices.map((choice) => JSON.stringify(choice)).join(', ')}`);
     }
-    if (option.defaultValue !== undefined && !option.negate) {
-      extraInfo.push(`default: ${option.defaultValueDescription || JSON.stringify(option.defaultValue)}`);
+    if (option.defaultValue !== undefined) {
+      // default for boolean and negated more for programmer than end user,
+      // but show true/false for boolean option as may be for hand-rolled env or config processing.
+      const showDefault = option.required || option.optional ||
+        (option.isBoolean() && typeof option.defaultValue === 'boolean');
+      if (showDefault) {
+        extraInfo.push(`default: ${option.defaultValueDescription || JSON.stringify(option.defaultValue)}`);
+      }
+    }
+    // preset for boolean and negated are more for programmer than end user
+    if (option.presetArg !== undefined && option.optional) {
+      extraInfo.push(`preset: ${JSON.stringify(option.presetArg)}`);
     }
     if (option.envVar !== undefined) {
       extraInfo.push(`env: ${option.envVar}`);

lib/option.js

@@ -28,6 +28,7 @@ class Option {
     }
     this.defaultValue = undefined;
     this.defaultValueDescription = undefined;
+    this.presetArg = undefined;
     this.envVar = undefined;
     this.parseArg = undefined;
     this.hidden = false;
@@ -48,6 +49,23 @@ class Option {
     return this;
   };
 
+  /**
+   * Preset to use when option used without option-argument, especially optional but also boolean and negated.
+   * The custom processing (parseArg) is called.
+   *
+   * @example
+   * new Option('--color').default('GREYSCALE').preset('RGB');
+   * new Option('--donate [amount]').preset('20').argParser(parseFloat);
+   *
+   * @param {any} arg
+   * @return {Option}
+   */
+
+  preset(arg) {
+    this.presetArg = arg;
+    return this;
+  };
+
   /**
    * Set environment variable to check for option value.
    * Priority order of option values is default < env < cli
@@ -166,6 +184,19 @@ class Option {
   is(arg) {
     return this.short === arg || this.long === arg;
   };
+
+  /**
+   * Return whether a boolean option.
+   *
+   * Options are one of boolean, negated, required argument, or optional argument.
+   *
+   * @return {boolean}
+   * @api private
+   */
+
+  isBoolean() {
+    return !this.required && !this.optional && !this.negate;
+  };
 }
 
 /**

tests/help.optionDescription.test.js

@@ -17,13 +17,33 @@ describe('optionDescription', () => {
     expect(helper.optionDescription(option)).toEqual(description);
   });
 
-  test('when option has default value then return description and default value', () => {
+  test('when boolean option has default value true then return description and default value', () => {
     const description = 'description';
-    const option = new commander.Option('-a', description).default('default');
+    const option = new commander.Option('-a', description).default(true);
     const helper = new commander.Help();
-    expect(helper.optionDescription(option)).toEqual('description (default: "default")');
+    expect(helper.optionDescription(option)).toEqual('description (default: true)');
   });
 
+  test('when boolean option has default value string then return description without default', () => {
+    const description = 'description';
+    const option = new commander.Option('-a', description).default('foo');
+    const helper = new commander.Help();
+    expect(helper.optionDescription(option)).toEqual('description');
+  });
+
+  test('when optional option has preset value then return description and default value', () => {
+    const description = 'description';
+    const option = new commander.Option('--aa [value]', description).preset('abc');
+    const helper = new commander.Help();
+    expect(helper.optionDescription(option)).toEqual('description (preset: "abc")');
+  });
+
+  test('when boolean option has preset value then return description without default', () => {
+    const description = 'description';
+    const option = new commander.Option('--bb', description).preset('abc');
+    const helper = new commander.Help();
+    expect(helper.optionDescription(option)).toEqual('description');
+  });
   test('when option has env then return description and env name', () => {
     const description = 'description';
     const option = new commander.Option('-a', description).env('ENV');
@@ -34,15 +54,15 @@ describe('optionDescription', () => {
   test('when option has default value description then return description and custom default description', () => {
     const description = 'description';
     const defaultValueDescription = 'custom';
-    const option = new commander.Option('-a', description).default('default value', defaultValueDescription);
+    const option = new commander.Option('-a <value>', description).default('default value', defaultValueDescription);
     const helper = new commander.Help();
     expect(helper.optionDescription(option)).toEqual(`description (default: ${defaultValueDescription})`);
   });
 
   test('when option has choices then return description and choices', () => {
     const description = 'description';
     const choices = ['one', 'two'];
-    const option = new commander.Option('-a', description).choices(choices);
+    const option = new commander.Option('-a <value>', description).choices(choices);
     const helper = new commander.Help();
     expect(helper.optionDescription(option)).toEqual('description (choices: "one", "two")');
   });

tests/options.bool.combo.test.js

@@ -96,28 +96,51 @@ describe('boolean option combo, default false, short flags', () => {
   });
 });
 
-// This is a somewhat undocumented special behaviour which appears in some examples.
-// When a flag has a non-boolean default, it is used as the value (only) when the flag is specified.
-//
-// boolean option combo with non-boolean default
+// boolean option combo with non-boolean default.
+// Changed behaviour to normal default in Commander 9.
 describe('boolean option combo with non-boolean default', () => {
-  test('when boolean combo not specified then value is undefined', () => {
-    const flagValue = 'red';
-    const program = createPepperProgramWithDefault(flagValue);
+  test('when boolean combo not specified then value is default', () => {
+    const program = createPepperProgramWithDefault('default');
     program.parse(['node', 'test']);
-    expect(program.opts().pepper).toBeUndefined();
+    expect(program.opts().pepper).toBe('default');
+  });
+
+  test('when boolean combo positive then value is true', () => {
+    const program = createPepperProgramWithDefault('default');
+    program.parse(['node', 'test', '--pepper']);
+    expect(program.opts().pepper).toBe(true);
+  });
+
+  test('when boolean combo negative then value is false', () => {
+    const program = createPepperProgramWithDefault('default');
+    program.parse(['node', 'test', '--no-pepper']);
+    expect(program.opts().pepper).toBe(false);
+  });
+});
+
+describe('boolean option combo with non-boolean default and preset', () => {
+  function createPepperProgramWithDefaultAndPreset() {
+    const program = new commander.Command();
+    program
+      .addOption(new commander.Option('-p, --pepper').default('default').preset('preset'))
+      .option('-P, --no-pepper', 'remove pepper');
+    return program;
+  }
+
+  test('when boolean combo not specified then value is default', () => {
+    const program = createPepperProgramWithDefaultAndPreset();
+    program.parse(['node', 'test']);
+    expect(program.opts().pepper).toBe('default');
   });
 
-  test('when boolean combo positive then value is "default" value', () => {
-    const flagValue = 'red';
-    const program = createPepperProgramWithDefault(flagValue);
+  test('when boolean combo positive then value is preset', () => {
+    const program = createPepperProgramWithDefaultAndPreset();
     program.parse(['node', 'test', '--pepper']);
-    expect(program.opts().pepper).toBe(flagValue);
+    expect(program.opts().pepper).toBe('preset');
   });
 
   test('when boolean combo negative then value is false', () => {
-    const flagValue = 'red';
-    const program = createPepperProgramWithDefault(flagValue);
+    const program = createPepperProgramWithDefaultAndPreset();
     program.parse(['node', 'test', '--no-pepper']);
     expect(program.opts().pepper).toBe(false);
   });

tests/options.bool.test.js

@@ -84,34 +84,33 @@ describe('boolean flag on command', () => {
   });
 });
 
-// This is a somewhat undocumented special behaviour which appears in some examples.
-// When a flag has a non-boolean default, it is used as the value (only) when the flag is specified.
-//
 // boolean flag with non-boolean default
+// NB: behaviour changed in Commander v9 to have default be default.
+// These tests no longer match likely uses, but retained and updated to match current behaviour.
 describe('boolean flag with non-boolean default', () => {
-  test('when flag not specified then value is undefined', () => {
+  test('when flag not specified then value is "default"', () => {
     const flagValue = 'black';
     const program = new commander.Command();
     program
-      .option('--olives', 'Add olives? Sorry we only have black.', flagValue);
+      .option('--olives', 'Add green olives?', flagValue);
     program.parse(['node', 'test']);
-    expect(program.opts().olives).toBeUndefined();
+    expect(program.opts().olives).toBe(flagValue);
   });
 
-  test('when flag specified then value is "default" value', () => {
+  test('when flag specified then value is true', () => {
     const flagValue = 'black';
     const program = new commander.Command();
     program
-      .option('-v, --olives', 'Add olives? Sorry we only have black.', flagValue);
+      .option('-v, --olives', 'Add green olives?', flagValue);
     program.parse(['node', 'test', '--olives']);
-    expect(program.opts().olives).toBe(flagValue);
+    expect(program.opts().olives).toBe(true);
   });
 
-  test('when flag implied and negated then value is false', () => {
+  test('when combo flag and negated then value is false', () => {
     const flagValue = 'black';
     const program = new commander.Command();
     program
-      .option('-v, --olives', 'Add olives? Sorry we only have black.', flagValue)
+      .option('-v, --olives', 'Add green olives?', flagValue)
       .option('--no-olives');
     program.parse(['node', 'test', '--olives', '--no-olives']);
     expect(program.opts().olives).toBe(false);