Make order not matter for adding combo positive/negative options (#2405)

Author: shadowspawn

Readme.md

@@ -277,10 +277,7 @@ cheese: stilton
 ### Other option types, negatable boolean and boolean|value
 
 You can define a boolean option long name with a leading `no-` to set the option value to false when used.
-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.
+Defined alone without a matching positive option, this also makes the option true by default.
 
 Example file: [options-negatable.js](./examples/options-negatable.js)
 

examples/options-negatable.js

@@ -22,6 +22,6 @@ console.log(`You ordered a pizza with ${sauceStr} and ${cheeseStr}`);
 
 // Try the following:
 //    node options-negatable.js
-//    node options-negatable.js --sauce
+//    node options-negatable.js --no-sauce
 //    node options-negatable.js --cheese=blue
 //    node options-negatable.js --no-sauce --no-cheese

lib/command.js

@@ -675,17 +675,7 @@ Expecting one of '${allowedValues.join("', '")}'`);
     const name = option.attributeName();
 
     // 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) {
+    if (option.defaultValue !== undefined) {
       this.setOptionValueWithSource(name, option.defaultValue, 'default');
     }
 
@@ -1126,7 +1116,29 @@ Expecting one of '${allowedValues.join("', '")}'`);
   }
 
   _prepareForParse() {
+    // Save the state the first time, then restore the state before each subsequent parse.
     if (this._savedState === null) {
+      // Do the special default of lone negated option to true, now that we have all the options.
+      // Filter for negated options that have not been processed already.
+      this.options
+        .filter(
+          (option) =>
+            option.negate &&
+            option.defaultValue === undefined &&
+            this.getOptionValue(option.attributeName()) === undefined,
+        )
+        .forEach((option) => {
+          // check for lone negated option: --no-foo without a --foo option
+          const positiveLongFlag = option.long.replace(/^--no-/, '--');
+          if (!this._findOption(positiveLongFlag)) {
+            this.setOptionValueWithSource(
+              option.attributeName(),
+              true,
+              'default',
+            );
+          }
+        });
+
       this.saveStateBeforeParse();
     } else {
       this.restoreStateBeforeParse();

tests/options.bool.combo.test.js

@@ -149,3 +149,43 @@ describe('boolean option combo with non-boolean default and preset', () => {
     expect(program.opts().pepper).toBe(false);
   });
 });
+
+describe('only lone negative causes implicit default of true', () => {
+  test('when lone negative then default value is true', () => {
+    const program = new commander.Command();
+    program.option('--no-pepper', 'remove pepper');
+    program.parse([], { from: 'user' });
+    expect(program.opts().pepper).toBe(true);
+  });
+
+  test('when boolean combo and negative first then no implicit default', () => {
+    // New behaviour in Commander 15, now only lone negative gets implicit default of true.
+    const program = new commander.Command();
+    program
+      .option('--no-pepper', 'remove pepper')
+      .option('--pepper', 'pepper only');
+    program.parse([], { from: 'user' });
+    expect(program.opts().pepper).toBeUndefined();
+  });
+
+  test('when boolean combo and negative second then no implicit default', () => {
+    const program = new commander.Command();
+    program
+      .option('--pepper', 'pepper only')
+      .option('--no-pepper', 'remove pepper');
+    program.parse([], { from: 'user' });
+    expect(program.opts().pepper).toBeUndefined();
+  });
+
+  test('when boolean combo and negative second with explicit default then get explicit default', () => {
+    // Prior to Commander 15, negative second would ignore implicit and explicit default value.
+    // Simpler rule now and the special default behaviour is for lone negative only. Add test since
+    // intentional change of behaviour for unlikely edge case.
+    const program = new commander.Command();
+    program
+      .option('--pepper', 'pepper only')
+      .option('--no-pepper', 'remove pepper', 'plain');
+    program.parse([], { from: 'user' });
+    expect(program.opts().pepper).toBe('plain');
+  });
+});