rails / thor

Thor is a toolkit for building powerful command-line interfaces.
http://whatisthor.com/
MIT License
5.12k stars 553 forks source link

Allow to suppress automatic creation of --no-boolean flags #733

Open xjunior opened 4 years ago

xjunior commented 4 years ago

Adds inverse: (false|SymbolString) option to suppress automatic creation of --no-[option] flags or customize the flag name.

Closes #417

xjunior commented 3 years ago

@rafaelfranca hello, can we get this in?

dorner commented 3 years ago

@xjunior there was some interest in being able to customize the inverse behavior. So rather than just "on/off" I'd like to see something like "inverse: 'inverse-name'" which would default to "no-{name}" but could be set to false to disable the inverse behavior entirely. Does that make sense?

xjunior commented 3 years ago

@dorner how does this look now?

xjunior commented 3 years ago

@dorner done and done

rafaelfranca commented 3 years ago

Don't this patch only change the usage output? I don't see the @inverse value being used to calculate the value.

xjunior commented 3 years ago

@rafaelfranca the inverse flags are really aesthetic apparently, as the actual option (true value) is what is really checked.

i.e.:

option :color, inverse: :no_color
...
use_color! if options[:color]
rafaelfranca commented 3 years ago

They do have behavior. Here for example. https://github.com/erikhuda/thor/blob/011dc48b5ea92767445b062f971664235973c8b4/lib/thor/parser/options.rb#L179. Your patch don't disable that behavior, only the help message.

G-Rath commented 3 years ago

Bit of a drive-by comment: once this is landed, would folks be open to supporting using :inverse on non-boolean types as well? A common use-case I have for this is with flags like --config, where I'd like to have --no-config to allow users to explicitly signify that the flag (including its default) should not be used (i.e "do not read any config file, even if the default .my-tools-config.json file is present")

Happy to make a new issue for this and even have a go at the PR, but figured it'd be better to wait until this is merged :)

G-Rath commented 3 years ago

@xjunior I'd also be open to helping with this PR, if you'd like a hand.

dorner commented 3 years ago

I'd be amenable to non-boolean inverse, but I'd like to see the PR to understand your approach.

xjunior commented 3 years ago

Quite honestly, my interest in this was sorely to disable the feature when necessary. Customizing the inverse name, and adding inverse for non-boolean flags touches the code a bit more as shown by @rafaelfranca. Unfortunately I haven't been having the capacity to work more on this, so if @G-Rath wants to pick this up, you're more than welcome :D Otherwise, I think I'm rolling back the changes to customize the name and keep the original intent of the PR. Thoughts?

dorner commented 3 years ago

I think the two pieces of functionality are separate enough that they should be two different PRs.

G-Rath commented 3 years ago

Customizing the inverse name, and adding inverse for non-boolean flags touches the code a bit more as shown I think the two pieces of functionality are separate enough that they should be two different PRs.

Completely agree - I think it would be best to land things in three sequential PRs:

  1. add inverse: true|false on boolean type flags to allow disabling the current --no-<flag> behaviour
  2. support inverse: true|false|<string> on boolean type flags to allow customising the inverse flag
  3. support inverse: true|false|<string> on non-boolean type flags

@dorner would you be open to doing it this way?

xjunior commented 3 years ago

@rafaelfranca

They do have behavior. Here for example. https://github.com/erikhuda/thor/blob/011dc48b5ea92767445b062f971664235973c8b4/lib/thor/parser/options.rb#L179. Your patch don't disable that behavior, only the help message.

I tried to follow the code, and I really couldn't see how it would influence the outcome. Can you provide an example?

monfresh commented 1 year ago

@rafaelfranca Following up on this. Could you please explain what you mean by the need to disable the behavior? Do you mean that if this option exists:

option :run, type: :boolean, default: false, inverse: false

then if someone tries to pass in the --no-run option, it should fail with an error like this one?

ERROR: "foo command" was called with arguments ["--no-run"]
Usage: "foo command"
rafaelfranca commented 1 year ago

then if someone tries to pass in the --no-run option, it should fail with an error like this one?

Yes, but only if no invalidation options are accepted. But mainly the value of run should not be false.

xjunior commented 9 months ago

Hello, @rafaelfranca. Would you give me some hints on how to implement this behavior?