ManageIQ / optimist

Optimist is a commandline option parser for Ruby that just gets out of your way.
https://manageiq.org/optimist/
MIT License
248 stars 36 forks source link

options that start with `no_` doesn't behave as expected #121

Open senhalil opened 3 years ago

senhalil commented 3 years ago

An option that starts with no_ which is false by default:

# file script.rb
require 'optimist'
opts = Optimist.options do
  opt :no_blank_cleaning, "Deactivates cleaning of null and empty fields", default: false
end
puts opts

Using the short-name -n does the opposite of using the long-name --no-blank-cleaning.

$ ruby script.rb -n
{:no_blank_cleaning=>false, :help=>false, :no_blank_cleaning_given=>true}
$ ruby script.rb --no-blank-cleaning
{:no_blank_cleaning=>true, :help=>false, :no_blank_cleaning_given=>true}

I suspect when the options name starts with no_ the logic interferes with the logic that handles the flags that are true by default. Since this is a flag which is false by default (even if its name starts with no_) I expected it to work like any other flag x which is false by default .

NickLaMuro commented 3 years ago

@senhalil Based on this commit:

https://github.com/ManageIQ/optimist/commit/4ee290fe5709425acb24519fe3e4ea96857e231b

This seems to be the desired behavior. There are a few things at play here, so I will try to explain the best I can:

Issues with the example

Issue 1: :name starts with no_

Since you are defining the flag starting with :no_, which is a special case in Optimist, this means --no-blank-cleaning is provided, it will instead set the value as true, instead of the normal false. This is handled the Optimist::Parser#parse here:

https://github.com/ManageIQ/optimist/blob/909bc7b9980e26ef3f6b18711319ca369df7e2eb/lib/optimist.rb#L252-L256

Which will eventually be inverted in Optimist::BooleanOption#parse:

https://github.com/ManageIQ/optimist/blob/909bc7b9980e26ef3f6b18711319ca369df7e2eb/lib/optimist.rb#L759-L761

Where self.name.to_s =~ /^no_/ will set the value to true if both the arg is a --no... and the name of the opt starts with no_.

Issue 2: Short flag mapping for BooleanOption

The second bit of the weirdness (but again, by design) is that you are using the short option that is auto-generated based off the name given. Resolution of default short options is done here:

https://github.com/ManageIQ/optimist/blob/909bc7b9980e26ef3f6b18711319ca369df7e2eb/lib/optimist.rb#L533-L544

Resolving a short option is very simplistic, and doesn't take into account the type of option that it is. So in your case, while :no_blank_cleaning is the name, and it defines the "negative" case, the short option always takes on the "positive" case of the two long form flags.

I wrote some tests in another PR to better illustrate this specifically:

https://github.com/ManageIQ/optimist/pull/122

with this commit specifically:

https://github.com/ManageIQ/optimist/pull/122/commits/2030af88ebf5e6ffed7e3bb4a3bd580a1cdf132b

Referring to my point here.

Workarounds/Alternatives

So in your case above, -n actually maps to --blank-cleaning since that flag is the "positive" case, but since you named your option :no_blank_cleaning, both -n and --blank-cleaning resolve to false.

I think there are two ways you can define your option where it would be more obvious to the user:

Option 1

Use short: :none in your opt definition:

opts = Optimist.options do
  opt :no_blank_cleaning, "Deactivates cleaning...", short: :none, default: false
end

This will make it so -n isn't defined, and there is less confusion as what is defined.

Option 2

Name your opt :blank_cleaning instead of naming it based on the negative case.

opts = Optimist.options do
  opt :blank_cleaning, "Clean null and empty fields", default: false
end

Personally, I think this option is more idiomatic of most command line applications, as -n usually wouldn't want to stand for a "negative case" of a option anyway (as there usually is more than one), so -b would be a better short option to mean "Activate cleaning".


I did myself find this confusing even to understand the rational behind https://github.com/ManageIQ/optimist/commit/4ee290fe5709425acb24519fe3e4ea96857e231b even, but once I did, I found myself agreeing with it in conceptually, so I don't think this is a bug.

Hope this helps.

kbrock commented 3 years ago

I do wonder if there is a way to change negative_given so it is only false if it is the negative of the argument given.

But I keep going back to option 2.

senhalil commented 3 years ago

When given a short form, it will take the logic of the "positive" (non-no) long flag.

I understand the explanation but I find it unintuitive. Short form and long form usually do the same actions in most cli tools. Short form is just a short way of writing the actual option (not a conditional version of it). This extra conditional reversal logic, is it really needed?

short-form (-x) does exactly what long-form (--XXX) does is more straightforward and corresponds to the idea of "shortcut". And by default includes the case where XXX starts with a o_

Fryguy commented 3 years ago

Note that this is mentioned in the FAQ here: https://github.com/ManageIQ/optimist/blob/master/FAQ.txt#L64-L92

I agree though that it's intuitive to have the short form be, well, a short form of the long form, and so I could understand this being a bug. If the requested option is --no-foo, then -n should mean --no-foo

But I keep going back to option 2.

option 2 is a workaround, but there is a clear bug here (maybe not a code bug, but a violation of Principle of Least Surprise)