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

Do not print default for "--no-" #736

Merged
merged 1 commit into from
Jan 6, 2018
Merged

Do not print default for "--no-" #736

merged 1 commit into from
Jan 6, 2018

Conversation

ntkme
Copy link
Contributor

@ntkme ntkme commented Jan 3, 2018

Consider the following case:

  Options:

     --no-color         Disable color. (default: true)

Internally the "default: true" would make sense because we normalize --no-color to color, so color would have default true.

However, from a user perspective it makes absolutely no sense. Users would interpret it as: --no-color has default true, which means color would be off by default, which is the opposite to what it is.

Let's assume we fix it by change it to print false, now look at the following case (#108 & #691):

  Options:

     --color            Enable color.
     --no-color         Disable color. (default: false)

In this case no matter which default it prints, it would be nothing but confusions.

So instead of printing default: false, I suggest to not print any default for --no-, and this PR would do that.

@roman-vanesyan roman-vanesyan merged commit f13ee18 into tj:master Jan 6, 2018
@roman-vanesyan
Copy link
Collaborator

Good catch

@ntkme ntkme deleted the fix-print-default branch January 7, 2018 01:49
@michael-crawford
Copy link

This issue seems to be related to something I'm trying to implement, but I'm not sure. I still can't seem to figure out a way to do what I think should be something simple, so not sure if this is the best place to ask, but as I think it's related, here goes:

Summary: How are we supposed to set a default value for a boolean option?

Two use cases. Default true vs default false.

  • options.monitor should default to false or undefined, but have a way to be set to true
  • options.check should default to true, but have a way to be set to false.

Current design as I understand it seems to support what I want as explicit command line options:

  • Add "--monitor" to CLI to get options.monitor = true, otherwise it's undefined
  • Add "--no-check" to CLI to get options.check = false, otherwise it's true

But I can't see how to make this work with default values, allowing me to set the default behavior via environment variables, but still allow for a command-line override.

  • If MONITOR env var exists and is 1 or true, options.monitor should default to true, and "--monitor" should have no effect. Otherwise, options.monitor should default to undefined or false, and "--monitor" would then set it to true.
  • If NOCHECK env var exists and is 1 or true, options.check should default to false, and "--check" should override this default to set it to true. Otherwise, options.check should default to true, and "--no-check" should override this and set it to false.

I thought this should work, but it doesn't. If there's not a way to do this, there should be.

let MONITOR = (process.env.MONITOR && (process.env.MONITOR == 'true' || process.env.MONITOR == '1')) ? true : false;
let CHECK = (process.env.NOCHECK && (process.env.NOCHECK == 'true' || process.env.NOCHECK == '1')) ? false : true;

program.command('test')
  .option('-m, --monitor', 'Monitor build process', MONITOR)
  .option('-C, --no-check', 'Do not check prerequisites', CHECK)

@shadowspawn
Copy link
Collaborator

Interesting question @michael-crawford and nice detailed explanation of what you are trying to achieve.

Would you please open a new issue for this? I think your issue is different to what this Pull Request is about and this is not the right place to discuss it. (You could perhaps add to #108, but I think your question is long enough and well constructed enough to go in its own issue.)

One thing you could add is example output to show what didn't work with what you tried at the bottom, so readers don't need to try the code to find out what goes wrong.

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

Successfully merging this pull request may close these issues.

5 participants