Refactor the part of .addOption() responsible for storing the default value

[aweebit]

Disclaimer: The change introduced here is of low importance.

The change has been manually cherry-picked from the now-closed #1915 so that it can be discussed independently.

Problem

When a default value is explicitly specified for the negating component of a negatable option (the one whose long flag is prefixed with --no-), it should be set as the default value for the option just like in case of the non-negating component (or a regular, non-negatable option).

That is exactly what happens in the current .addOption() code except for one case, namely when the non-negating component had already been added earlier. To check if that is the case, ._findOption() has to be called, and that seems excessive.

Demo

const program = new Command()
  .option('--cheese <flavour>', 'cheese flavour', 'mozzarella')
  .option('--no-cheese', 'plain with no cheese', 'none') // 'none' simply ignored
  .parse();
console.log(program.opts().cheese); // mozzarella

Solution

Eliminate excessive ._findOption() calls and undefined checks.

The change can be considered breaking because none is logged when running the demo program from above after it is applied, so might want to include it in the next major release. However, code similar to the demo is so unlikely to have ever been written that I don't think caring about it is worth it.

[shadowspawn]

Eliminate excessive ._findOption() calls and undefined checks.

I wonder whether you discovered there was a reason for the ._findOption call, as you do not appear to have eliminated it after all?

For historical interest: the PR to make combination of positive and negative options actually sensible was #795. This is where the bonus .findOption() call comes from. (The modern code looks very different due to changes since then, but evolution from that PR.)

[aweebit]

I wonder whether you discovered there was a reason for the ._findOption call, as you do not appear to have eliminated it after all?

The comment says it all:

// --no-foo is special and defaults foo to true, unless a --foo option is already defined

So I have eliminated the call in the case when a default value is explicitly specified for the negating component, but if it's not, the call is still necessary so that true is only set as the default value if the non-negating component hasn't been added yet.

[shadowspawn]

When a default value is explicitly specified for the negating component of a negatable option (the one whose long flag is prefixed with --no-), it should be set as the default value for the option just like in case of the non-negating component (or a regular, non-negatable option).

That is not the behaviour I decided on and documented. The README says:

If you define --foo first, adding --no-foo does not change the default value from what it would
otherwise be.

I deliberately treated the implicit default of true and an explicit user-supplied default the same way, and followed the existing behaviour for the implicit default. The default is applied if negated option specified alone, and it is ignored if the positive option comes first.

[shadowspawn]

The current behaviour is By Design, so there would need to be a compelling case for a change in behaviour.