fixes behavior of --no-* options

[usmonster]

A bug existed whenever a --no-prefixed option was defined along with
its counterpart (e.g. --status and --no-status), which caused the
corresponding boolean attribute to always be false when either option
was specified on the command line.

This was because the definition of a negating option would overwrite the
default value of the attribute, combined with the fact that specifying either
option would emit two events with conflicting logic.

This has been corrected in two ways:

1. During the initialization of a negating option, we now check to see if the
   non-negated counterpart was already defined, in which case we don't
   set/overwrite the default value (which would otherwise be set to true);
2. An option's name no longer omits its negation prefix; that is,
   "--no-cheese" is now internally named "no-cheese", which will distinguish
   it from "--cheese". This will allow unique listeners & events to be registered and
   emitted, depending on which option is passed to the command, thus
   avoiding any attribute assignment collision/race.

Additionally, the tests for negated options were updated to more
explicitly demonstrate the expected behavior, and a couple of relevant
examples were fixed to match their intended behavior.

[usmonster]

Note: This may be a breaking change for people who use the (only recently-documented) Custom Event Listeners, though I can also clarify this by adding another illustrative example to the Readme in that section, if required.

[usmonster]

Bump–@vanesyan, does this look OK for you?

[roman-vanesyan]

Yes, everything is ok for me, but cannot merge this until the next major bump as it's breaking change.

[shadowspawn]

Good description of problem and changes, thanks @usmonster

I am hopeful this will tidy up --no- options! I have done a visual inspection of changes. I have not tested runtime yet.

Also good catch that the JSDoc was claiming default value was true rather than undefined, oops.

[shadowspawn]

@usmonster
Thanks for updating PR, and comments in #939.

The change to the custom event listener makes this a breaking change for a major version, so I have been working on some other pull requests first.

Good to know you are still around and interested, makes this one high on my list. :-)

[shadowspawn]

(In top two of my PR queue. Hopefully take another look this week.)

[shadowspawn]

I have been trying all the permutations by running programs to check behaviour, as the code itself is quite subtle. One request, one comment, one thing I am still wondering about, one future change. (Sorry, I do some long comments!)

Overall, I think this will make --no- useful and usable in combinations, and worth the pain of a breaking change. Thanks! I think the original vision was just for a stand-alone negated boolean, but people quite reasonably want and try to use it in combination with all forms of options. And are disappointed.

1. Request: I think in practice the option with a required value (e.g. <value>) should be people's first choice, and the option with optional value (e.g. [value]) is less useful and harder to handle. So for the pizza example, I would like cheese to be required rather than optional. (But the existing example and output are inconsistent, well spotted!)

2. Musing: I am wondering about name in opts. This is the only use of operator in and stuck out to me. Would testing .optionFor be appropriate and more consistent with existing code?

3. Breaking note. This is a breaking change to the default values of options when no command line arguments are supplied (and other situations, but this is the big one). I think the new behaviour is a reasonable one to explain, simple story: declaring --no-foo after --foo does not change default value of foo.

const program = require("commander");

program
  .option('-c, --cheese <type>', 'Add the specified type of cheese', "marble")
  .option('-C, --no-cheese', 'You do not want any cheese')
  .option('--what <type>')
  .option('--no-what');

program.parse(process.argv);
console.log(`cheese is ${program.cheese}`);
console.log(`what is ${program.what}`);

# Before:
$ node required.js
cheese is true
what is true

# After:
$ node required.js
cheese is marble
what is undefined

4. Future: I think we should allow a default value for pure flags. This would allow an externally supplied default which can be overridden on command line. Fixing up --no-* is first though and better to deal with one thing at a time, so I am not suggesting you do this in this PR, but it was on my mind.

program
  .option('--what <type>', 'enable on command line', process.env.WHAT)
  .option('--no-what', 'disable on command line');

[slavafomin]

Please, take a look at my comment here.

[usmonster]

Thanks for the comment, @slavafomin. I replied in the thread.

Belated thanks, @shadowspawn, for the thorough review and feedback! No need to apologize for long comments, as they are appreciated. :) I'll reply to each below:

1. Totally agree, updated the example!
2. I'm all for consistency, but .optionFor doesn't really work in a way that makes it useful in this case. I think name in opts is exactly the right tool for the job in this case, so I'll leave it as-is, if you don't mind?
3. Just to clarify, would you like me to document a new example in the readme, or update my commit message to include your example, or is it enough to just add a comment to the pizza example with the new expected output? Personally, I think the changelog/release notes should be enough if you prefix the commit subject with [BC BREAK], but OK for me if you also want something more explicitly documenting the new behavior.
4. I understand what you mean, and I read through Support defaults for boolean options (including --no-*), for env var set defaults #928 to see the use cases, but I'm still not convinced about its utility. Based on the example you gave in the above comment, I think there's already a pretty straightforward way to do that:

const handleWhat = function() {
  // negate `what` if default isn't `true`
  if ('WHAT' in process.env && process.env.WHAT !== 'true') {
    this.what = !this.what;
  }
};

program
  .option('--what', 'enable on command line')
  .option('--no-what', 'disable on command line')
  .on('option:what', handleWhat)
  .on('option:no-what', handleWhat)
  .parse(process.argv);

console.log(`what is ${program.what}`);

It can even be done without event listeners by doing the conditional reassignment at some point after the .parse (example in my comment). In any case, if enough people want this to be a built-in feature, I'll let someone else make that PR, but I'm kinda not for it. ;)

Thanks in advance for your feedback, especially on my question in point 3. :)

[shadowspawn]

1. Thanks. :-)

2. What about {}.hasOwnProperty.call(opts, name)instead then? I am paranoid about (more) conflicts with built-in properties. (I have not tested runtime, suggesting this in theory.)

e.g. https://eslint.org/docs/rules/guard-for-in

3. No changes from you needed thanks. I was thinking out loud and making a note to myself, it isn't just the event listener change that is breaking. I'll update the README section on options and comfortable doing that myself after carefully crafting the current coverage.

4. I have the ball with that one. My example was not very good. I might invite you to review if I get a candidate Pull Request done for v3. 😄

[shadowspawn]

Thanks for switching base branch! I have not been asking people to do that in case it creates problems, but does make it easier for me. :-)

[shadowspawn]

Starting merge.

[shadowspawn]

Updated example and README with new improved behaviour, and added entries to CHANGELOG

This is one of my favourite changes in v3. 😄

Thank you for your contributions.

[usmonster]

Thanks for your proactive involvement!

[shadowspawn]

Available now as a prerelease. See #1001

[shadowspawn]

Shipped in v3: https://github.com/tj/commander.js/releases/tag/v3.0.0