Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework option defaults and add preset #1652

Merged
merged 24 commits into from Dec 21, 2021

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Dec 12, 2021

Pull Request

Problem

  1. Using "default" value with boolean options and optional options is a bit fragile and buggy.
  • non-boolean default used with boolean option is intended to replace "true" when option used, but different behaviour than normal default, never documented in README, and bugs when option repeated
  • default used with optional does set the default, but breaks the use of optional without an option argument which is just ignored
  • behaviour varies depending on previous value
  1. No way to set value for optional used without argument .

  2. Optional used without argument does not trigger custom processing, complicating some uses.

  3. This area of code is a problem for adding features to the code with some difficult to understand behaviours and intentions.

Matching issue: #1648

Related: #116 #440 #928 #984 #1135 #1201 #1328 #1394

Solution

  1. Make default consistently set just the default (initial) value for boolean, required, and optional options. Fix use of optional with a default value.

  2. Add Option.preset() for explicitly setting the preset value to use instead of true with a boolean option, or optional without an option argument.

  3. Using preset does trigger custom processing.

ChangeLog

  • fixed: option with optional argument not supplied on command line now works when option already has a value, whether from default value or from previous arguments
  • changed: default value specified for boolean option now always used as default value (see .preset() to match some previous behaviours)
  • changed: default value for boolean option only shown in help if true/false
  • added: Option.preset() allows specifying value/arg for option when used without option-argument (especially optional, but also boolean option)

@shadowspawn shadowspawn added the semver: major label Dec 12, 2021
@shadowspawn shadowspawn added this to the Commander v9.0.0 milestone Dec 12, 2021
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Dec 12, 2021

The core of the processing dates all the way back to at least v1.1.0. For interest:

v1.1.0 code setting default value (note, not including boolean):

// preassign default value only for --no-*, [optional], or <required>
if (false == option.bool || option.optional || option.required) {
    // when --no-* we make sure default is true
    if (false == option.bool) defaultValue = true;
    // preassign only if we have a default
    if (undefined !== defaultValue) self[name] = defaultValue;
 }

v1.1.0 code setting new option value. Note: null and undefined val. Default value being used in multiple places. Choosing behaviour based on previous value. Tricky!

    if (null !== val && fn) val = fn(val, undefined === self[name]
      ? defaultValue
      : self[name]);

    // unassigned or bool
    if ('boolean' == typeof self[name] || 'undefined' == typeof self[name]) {
      // if no value, bool true, and we have a default, then use it!
      if (null == val) {
        self[name] = option.bool
          ? defaultValue || true
          : false;
      } else {
        self[name] = val;
      }
    } else if (null !== val) {
      // reassign
      self[name] = val;
    }
    /// else ignoring optional so value unchanged
  });

@shadowspawn shadowspawn changed the title Feature/optional value Rework option defaults Dec 12, 2021
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Dec 13, 2021

This PR changes the boolean behaviour with defaults, which is a breaking change. The use of defaults with boolean options has not been much documented so hopefully not in widespread use.

Long version of behaviours.

program
  .option('--plain', 'option without a default')
  .option('--default-false', 'option with a boolean default', false)
  .option('--default-string', 'option with a non-boolean default', 'stringy');

program.parse(process.argv);
console.log(program.opts());

commander@2

  • "default" is not used as default
  • double option with non-boolean default returns to undefined
% node bug1
{ plain: undefined, defaultFalse: undefined, defaultString: undefined }
% node bug1 --plain --default-false --default-string  
{ plain: true, defaultFalse: true, defaultString: 'stringy' }
% node bug1 --plain --plain --default-false --default-false --default-string --default-string
{ plain: true, defaultFalse: true, defaultString: undefined }

commander@8

  • "default" is not used as default, except for boolean values
  • double option with non-boolean default returns to undefined
% node bug1                                                                                 
{ defaultFalse: false }
% node bug1 --plain --default-false --default-string                                        
{ defaultFalse: true, plain: true, defaultString: 'stringy' }
% node bug1 --plain --plain --default-false --default-false --default-string --default-string
{ defaultFalse: true, plain: true, defaultString: undefined }

This PR.

  • "default" is used as default
% node bug1                                                                                 
{ defaultFalse: false, defaultString: 'stringy' }
% node bug1 --plain --default-false --default-string                                        
{ defaultFalse: true, defaultString: true, plain: true }
% node bug1 --plain --plain --default-false --default-false --default-string --default-string
{ defaultFalse: true, defaultString: true, plain: true }

And new behaviour available with preset.

program.addOption(new Option('--preset-string').preset('stringy'));
% node  bug1
{}
% node  bug1 --preset-string
{ presetString: 'stringy' }
% node  bug1 --preset-string --preset-string
{ presetString: 'stringy' }

@shadowspawn shadowspawn changed the title Rework option defaults Rework option defaults and add preset Dec 17, 2021
@shadowspawn shadowspawn changed the base branch from develop to release/9.x Dec 18, 2021
@shadowspawn shadowspawn marked this pull request as ready for review Dec 18, 2021
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Dec 18, 2021

I should probably add the preset to the usage in the help?

  -t, --timeout <delay>  timeout in seconds (default: one minute)
  -d, --drink <size>     drink cup size (choices: "small", "medium", "large")
  -p, --port <number>    port number (env: PORT)
  --donate [amount]      (preset: 20)

Edit: Done.

@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Dec 20, 2021

This PR fixes the behaviour of optional used without an option-argument, so it works when option already has a value whether from default or from previous arguments on command line.

program
  .option('-o, --optional [value]')
  .option('-p [value]', 'description', 'default');

// program.P = 'oops';

program.parse(process.argv);
console.log(program.opts());

Commander 8

% node bug2.js           
{ p: 'default' }
% node bug2.js -o -p 
{ p: 'default', optional: true } # BUG
% node bug2.js -o cli -p cli
{ p: 'cli', optional: 'cli' }
% node bug2.js -o cli -p cli -o -p
{ p: 'cli', optional: 'cli' } #BUG

Behaviour with this PR

% node bug2.js                    
{ p: 'default' }
% node bug2.js -o -p              
{ p: true, optional: true }
% node bug2.js -o cli -p cli      
{ p: 'cli', optional: 'cli' }
% node bug2.js -o cli -p cli -o -p
{ p: true, optional: true }

Copy link
Collaborator

@abetomo abetomo left a comment

Great work!

@shadowspawn shadowspawn merged commit 9a56cc7 into tj:release/9.x Dec 21, 2021
12 checks passed
@shadowspawn shadowspawn deleted the feature/optional-value branch Dec 21, 2021
@shadowspawn shadowspawn added the pending release label Dec 21, 2021
@shadowspawn shadowspawn removed the pending release label Jan 29, 2022
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jan 29, 2022

Commander v9 has been released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants