Make order of negated options irrelevant

[micheleb]

Hey there! 👋

Thanks for this great library. I've been using it for years, and it's really amazing. :)

It would be really nice if the order in which options and their negations are specified didn't matter, since it's an invisible requirement that easily breaks when you want your options to be alphabetically sorted.

I was able to fix the issue with the patch below. Would something like this be considered for inclusion in the library? I can create a PR in that case, of course.

Thanks!

diff --git a/node_modules/commander/lib/command.js b/node_modules/commander/lib/command.js
index efbb8f6..5c74f6a 100644
--- a/node_modules/commander/lib/command.js
+++ b/node_modules/commander/lib/command.js
@@ -659,15 +659,43 @@ Expecting one of '${allowedValues.join("', '")}'`);
     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)) {
+      const positiveOption = this._findOption(positiveLongFlag);
+      if (!positiveOption) {
         this.setOptionValueWithSource(
           name,
           option.defaultValue === undefined ? true : option.defaultValue,
           'default',
         );
+      } else {
+        // If positive option exists and was auto-defaulted to true due to this negative option being added later,
+        // remove that auto-default to prevent {foo: true} when neither --foo nor --no-foo is used
+        const positiveOptionName = positiveOption.attributeName();
+        if (this.getOptionValueSource(positiveOptionName) === 'default' && 
+            this.getOptionValue(positiveOptionName) === true &&
+            positiveOption.defaultValue === undefined) {
+          // Remove the auto-set default
+          delete this._optionValues[positiveOptionName];
+          delete this._optionValueSources[positiveOptionName];
+        }
       }
     } else if (option.defaultValue !== undefined) {
       this.setOptionValueWithSource(name, option.defaultValue, 'default');
+    } else {
+      // Check if this positive option has a corresponding negative option that was added first
+      // and auto-set a default value. If so, remove that auto-default.
+      const negativeLongFlag = option.long ? option.long.replace(/^--/, '--no-') : null;
+      const negativeOption = negativeLongFlag && this._findOption(negativeLongFlag);
+      if (negativeOption && negativeOption.negate) {
+        // If we already have a default value set for this option due to the negative option,
+        // and the negative option didn't have an explicit default, remove the auto-default
+        if (this.getOptionValueSource(name) === 'default' && 
+            this.getOptionValue(name) === true &&
+            negativeOption.defaultValue === undefined) {
+          // Remove the auto-set default
+          delete this._optionValues[name];
+          delete this._optionValueSources[name];
+        }
+      }
     }
 
     // handler for cli and env supplied values

[shadowspawn]

Thanks for this great library. I've been using it for years, and it's really amazing. :)

Thanks!

It would be really nice if the order in which options and their negations are specified didn't matter,

Hmm... I agree in principle, but don't like the amount of code or reversing previous changes. I'll keep thinking about this.

(For historical interest, the current approach comes from #795 and much improved previous behaviour covered in #939)

you want your options to be alphabetically sorted.

You might be referring to in the source code, but for the displayed help you can get Commander to do the sorting with sortOptions: https://github.com/tj/commander.js/blob/master/docs/help-in-depth.md

[shadowspawn]

// If positive option exists and was auto-defaulted to true due to this negative option being added later,
// remove that auto-default to prevent {foo: true} when neither --foo nor --no-foo is used

I don't think this code will ever get called, as the negative option being added later is what is being processed right now, and we just avoided overwriting the default value.

[micheleb]

Thanks for responding so quickly!

I don't think this code will ever get called, as the negative option being added later is what is being processed right now, and we just avoided overwriting the default value.

Indeed. Sorry, I posted the diff from patch-package that solved my issue by fiddling with the library in node_modules. I just wanted to check if it was something that would even be considered before doing it properly with a PR, tests, and everything. But now that I know it might get included, I’ve done my homework :)

You might be referring to in the source code, but for the displayed help you can get Commander to do the sorting with sortOptions: https://github.com/tj/commander.js/blob/master/docs/help-in-depth.md

I'm actually using sortOptions (and sortSubcommands) in one of the two bigger projects where I use commander.js! :) But in the other, I’ve built a bit of a cathedral on top of commander and inquirer, so I need to apply some custom sorting logic to group options, and sortOptions isn’t enough in that case.