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

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

Open
senhalil opened this issue Jul 29, 2021 · 4 comments
Open

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

senhalil opened this issue Jul 29, 2021 · 4 comments
Assignees
Labels

Comments

@senhalil
Copy link

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

NickLaMuro commented Aug 16, 2021

@senhalil Based on this commit:

4ee290f

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:

optimist/lib/optimist.rb

Lines 252 to 256 in 909bc7b

arg, negative_given = if arg =~ /^--no-([^-]\S*)$/
["--#{$1}", true]
else
[arg, false]
end

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

optimist/lib/optimist.rb

Lines 759 to 761 in 909bc7b

def parse(_paramlist, neg_given)
return(self.name.to_s =~ /^no_/ ? neg_given : !neg_given)
end

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:

optimist/lib/optimist.rb

Lines 533 to 544 in 909bc7b

def resolve_default_short_options!
@order.each do |type, name|
opts = @specs[name]
next if type != :opt || opts.short
c = opts.long.split(//).find { |d| d !~ INVALID_SHORT_ARG_REGEX && !@short.member?(d) }
if c # found a character to use
opts.short = c
@short[c] = name
end
end
end

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:

#122

with this commit specifically:

2030af8

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 4ee290f 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
Copy link
Member

kbrock commented Aug 17, 2021

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

senhalil commented Oct 1, 2021

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

Fryguy commented Oct 1, 2021

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)

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

No branches or pull requests

5 participants