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 to suppress automatic creation of --no-boolean flags #733

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

xjunior
Copy link

@xjunior xjunior commented Jul 25, 2020

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

Closes #417

@xjunior
Copy link
Author

xjunior commented Mar 26, 2021

@rafaelfranca hello, can we get this in?

@dorner
Copy link

dorner commented Jun 1, 2021

@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
Copy link
Author

xjunior commented Jun 1, 2021

@dorner how does this look now?

Copy link

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Looking good! Couple of suggestions.

lib/thor.rb Outdated Show resolved Hide resolved
lib/thor/parser/option.rb Outdated Show resolved Hide resolved
@xjunior xjunior force-pushed the allow-bollean-inversion-suppression branch from ac1f35f to f9c1bdc Compare June 1, 2021 18:56
@xjunior
Copy link
Author

xjunior commented Jun 1, 2021

@dorner done and done

Copy link

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Much nicer! @rafaelfranca you good?

@rafaelfranca
Copy link
Member

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

@xjunior
Copy link
Author

xjunior commented Jun 3, 2021

@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
Copy link
Member

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.

lib/thor.rb Outdated Show resolved Hide resolved
@G-Rath
Copy link

G-Rath commented Jun 29, 2021

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
Copy link

G-Rath commented Jun 29, 2021

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

@dorner
Copy link

dorner commented Jun 29, 2021

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

Co-authored-by: Gareth Jones <Jones258@Gmail.com>
@xjunior
Copy link
Author

xjunior commented Jun 29, 2021

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
Copy link

dorner commented Jun 29, 2021

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

@G-Rath
Copy link

G-Rath commented Jun 29, 2021

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
Copy link
Author

xjunior commented Aug 26, 2021

@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
Copy link

@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
Copy link
Member

rafaelfranca commented May 12, 2023

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
Copy link
Author

xjunior commented Nov 23, 2023

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

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.

Allow suppression of auto-created 'no-' flags for boolean options
5 participants