Allow boolean default for flag option

[shadowspawn]

(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]

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]

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

[usmonster]

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]

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

[abetomo]

👍

[usmonster]

@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]

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]

Thanks for review @abetomo.

[usmonster]

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]

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

[shadowspawn]

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

[usmonster]

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).

[shadowspawn]

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]

Available now as a prerelease. See #1001

[shadowspawn]

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