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

Allow boolean default for flag option #987

Merged

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Jun 30, 2019

(Have not added anything to README yet, but code and tests as intended.)

This allows supplying default value for boolean flags, in particular for a default value when both --foo and --no-foo options are defined, such as a default value from environment. Only possible after #795.

Resolves #928.

const program = require("commander");

var colourDefault;
if (process.env.COLOUR === "0" || process.env.COLOUR === "false")
  colourDefault = false;
if (process.env.COLOUR === "1" || process.env.COLOUR === "true")
    colourDefault = true;

program
  .option('-c,--colour', 'enable colour', colourDefault)
  .option('-C,--no-colour', 'disable colour');

program.parse(process.argv);

console.log(`colour is ${program.colour}`);
$ node index.js -h
Usage: index [options]

Options:
  -c,--colour     enable colour
  -C,--no-colour  disable colour
  -h, --help      output usage information
$ node index.js 
colour is undefined
$ node index.js -C
colour is false
$ node index.js -c
colour is true

$ COLOUR=1 node index.js -h
Usage: index [options]

Options:
  -c,--colour     enable colour (default: true)
  -C,--no-colour  disable colour
  -h, --help      output usage information
$ COLOUR=1 node index.js 
colour is true
$ COLOUR=1 node index.js -C
colour is false
$ COLOUR=1 node index.js -c
colour is true

$ COLOUR=0 node index.js -h
Usage: index [options]

Options:
  -c,--colour     enable colour (default: false)
  -C,--no-colour  disable colour
  -h, --help      output usage information
$ COLOUR=0 node index.js 
colour is false
$ COLOUR=0 node index.js -c
colour is true
$ COLOUR=0 node index.js -C
colour is false

@shadowspawn shadowspawn added this to the v3.0.0 milestone Jun 30, 2019
@shadowspawn shadowspawn changed the base branch from release/3.0.0 to master July 1, 2019 06:31
@shadowspawn shadowspawn changed the base branch from master to release/3.0.0 July 1, 2019 06:31
@shadowspawn shadowspawn changed the title [WIP] Feature/928 allow boolean default for flag option Feature/928 allow boolean default for flag option Jul 1, 2019
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Jul 1, 2019

To clarify one breaking aspect of this change.

There is a subtle and I think undocumented behaviour I only discovered working through this code that you can set a value for a flag to take when specified (instead of true). This may even be completely unintentional. This behaviour is unchanged by this Pull Request for a non-boolean value.

program.option('--capital', 'set capital', 'Wellington');
...
console.log(program.capital);
$ example
undefined
$ example --capital
Wellington

So there was an existing behaviour that these two calls were actually the same, and specifying a boolean default of true would have no affect previously, and now it will have an affect:

program.option('--silly', 'desc');
program.option('--silly', 'desc', true);

I think the new behaviour is reasonable, and the old behaviour is undocumented and perhaps even accidental, but the change did break one of the existing tests.

@shadowspawn
Copy link
Collaborator Author

@usmonster FYI, the future change I mentioned in #795.

@shadowspawn shadowspawn marked this pull request as ready for review July 1, 2019 08:24
@shadowspawn shadowspawn changed the title Feature/928 allow boolean default for flag option Allow boolean default for flag option Jul 1, 2019
@usmonster
Copy link
Contributor

Thanks for the ping, @shadowspawn. FYI, I'd like to review this, but I probably can't before the end of the week.

Regarding the old behavior you mention, I'm pretty sure it was intentional to allow any option (even flags) to have a default value specified, especially given that there's a test for it. I imagine some folks may rely on this behavior even if undocumented, but your change does make sense as well.

@shadowspawn
Copy link
Collaborator Author

I imagine some folks may rely on this behavior even if undocumented

Yes indeed. I actually looked up the following quote when I joined as a maintainer and have been expecting an opportunity to use it and/or suffer from it. :-)

With a sufficient number of users of an API,
it does not matter what you promise in the contract:
all observable behaviors of your system
will be depended on by somebody.

http://www.hyrumslaw.com

Copy link
Collaborator

@abetomo abetomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@usmonster
Copy link
Contributor

@shadowspawn, my only concern is that I'm not sure how many users would prefer the new feature over the existing behavior, or vice-versa. If you have more data on this beyond the initial request, feel free to share. I just don't want more issues to pop up over this breaking change.

@shadowspawn
Copy link
Collaborator Author

I do not have any metrics on preferred behaviour. I do like making it convenient to have an externally supplied default that can be overridden, with--foo/--no-foo actually functional (thanks!).

From code inspection and experimentation I think the old behaviour was that a boolean default value (third parameter) was ignored, which I do see a use case for?

const program = require("commander");

program
  .option("-t,--truthy", "truthy", true)
  .option("-f,--falsey", "falsey", false)
  .option("-s,--surprise", "surprise", "surprise");

program.parse(process.argv);

console.log(`truthy ${program.truthy}`);
console.log(`falsey ${program.falsey}`);
console.log(`surprise ${program.surprise}`);

Old behaviour:

$ node index.js --help
Usage: index [options]

Options:
  -t,--truthy    truthy
  -f,--falsey    falsey
  -s,--surprise  surprise
  -h, --help     output usage information
$ node index.js 
truthy undefined
falsey undefined
surprise undefined
$ node index.js -t -f -s
truthy true
falsey true
surprise surprise

New behaviour. (Reminder, the intended use case is for --foo/--no-foo but highlighting change in behaviour.)

$ node index.js --help
Usage: index [options]

Options:
  -t,--truthy    truthy (default: true)
  -f,--falsey    falsey (default: false)
  -s,--surprise  surprise
  -h, --help     output usage information
$ node index.js 
truthy true
falsey false
surprise undefined
$ node index.js -t -f -s
truthy true
falsey true
surprise surprise

@shadowspawn
Copy link
Collaborator Author

Thanks for review @abetomo.

@usmonster
Copy link
Contributor

Thanks for the further clarification, @shadowspawn. I misunderstood the current behavior. I'm on board with this change, but will leave an inline comment or two.

@shadowspawn
Copy link
Collaborator Author

Expanded tests to have more complete coverage of intended boolean flag permutations.

@shadowspawn
Copy link
Collaborator Author

Added written description of possibility for default value for --foo/--no-foo to README.

@@ -101,8 +101,9 @@ cheese: stilton

You can specify a boolean option long name with a leading `no-` to set the option value to false when used.
Defined alone this also makes the option true by default.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In rereading this and the edits, I'm not sure we're using consistent vocabulary on this specific line; i.e. it can be unclear whether "define" (or "specify", "add", "set", etc.) means "when defining the program" or "when invoking the program via the CLI."

It could just be that the sentence is slightly awkwardly worded (and sorry to comment on something that isn't a change in this commit).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the awkward and potential for misinterpretation. I have struggled with the wording a couple of times, but at least we do have examples to consult. I'm not planning to have another go in this PR.

@shadowspawn shadowspawn merged commit a9503bb into tj:release/3.0.0 Jul 25, 2019
@shadowspawn shadowspawn added the pending release Merged into a branch for a future release, but not released yet label Jul 25, 2019
shadowspawn added a commit that referenced this pull request Jul 25, 2019
@shadowspawn
Copy link
Collaborator Author

Available now as a prerelease. See #1001

@shadowspawn shadowspawn deleted the feature/928-allow-boolean-default branch July 27, 2019 07:40
@shadowspawn
Copy link
Collaborator Author

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

Successfully merging this pull request may close these issues.

3 participants