Skip to content

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

Closed
aweebit wants to merge 1 commit intotj:developfrom
aweebit:refactor/addOption-default-value
Closed

Refactor the part of .addOption() responsible for storing the default value#1961
aweebit wants to merge 1 commit intotj:developfrom
aweebit:refactor/addOption-default-value

Conversation

@aweebit
Copy link
Copy Markdown
Contributor

@aweebit aweebit commented Aug 13, 2023

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.

@aweebit
Refactor addOption() part storing default value
d1a1b39 
Eliminate excessive ._findOption() calls and undefined checks.

Has the tiny side effect of overwriting the default value of a negatable
option when its negating component prefixed with "--no-" is added if
that component has an explicitly specified default value.
@aweebit aweebit mentioned this pull request Aug 13, 2023
Closed
@aweebit aweebit mentioned this pull request Aug 13, 2023
Closed
@shadowspawn
Copy link
Copy Markdown
Collaborator

shadowspawn commented Aug 20, 2023

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
Copy link
Copy Markdown
Contributor Author

aweebit commented Aug 20, 2023

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
Copy link
Copy Markdown
Collaborator

shadowspawn commented Aug 21, 2023 •
edited
Loading

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.

angelaschultz0622

This comment was marked as duplicate.

Sign in to view

@shadowspawn
Copy link
Copy Markdown
Collaborator

shadowspawn commented Sep 10, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@angelaschultz0622 angelaschultz0622 angelaschultz0622 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aweebit @shadowspawn @angelaschultz0622