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

multiple=True for an option-type argument also sets max_values > 1? #415

Closed
birkenfeld opened this issue Feb 4, 2016 · 7 comments
Closed
Labels
A-docs Area: documentation, including docs.rs, readme, examples, etc...

Comments

@birkenfeld
Copy link
Contributor

Here is a small demo for the issue. Take this code:

extern crate clap;

use clap::{App, Arg, ArgMatches};

fn get_arguments<'a>() -> ArgMatches<'a> {
    App::new("app")
        .version("1.0")
        .arg(Arg::with_name("option")
                 .takes_value(true)
                 //.multiple(true)
                 //.max_values(1)
                 .short("O"))
        .arg(Arg::with_name("positional")
                 .takes_value(true)
                 .required(true))
        .get_matches()
}

fn main() {
    let arguments = get_arguments();
}

which should run fine with arguments -O x pos.
When you uncomment the multiple(true), it should behave the same (I assume from the docs), but instead it complains that the positional arg is not given, which leads me to believe that the option now accepts multiple values. And indeed, also uncommenting the max_values(1) makes it behave again.

I hope this isn't expected behavior -- max_values > 1 arguments are thoroughly confusing in my opinion.

@kbknapp
Copy link
Member

kbknapp commented Feb 4, 2016

@birkenfeld This is expected behavior and should be corrected in the documentation if it's unclear, which sadly is hard to tell from my optic because I'm familiar with all the internals so I take a lot of that knowledge for granted.

Just to ensure we're speaking about the same thing. You'd like multiple(true) to only mean, -O val1 -O val2 is allowed, but stop parsing for values after the first value (per occurrence of -O), and continue along not the parsing chain not assuming another value to -O is coming next? If that's the case, you have to set number_of_values(1) to get that effect.

Explanation below. Bare with me here, explaining multiples gets hairy :)


First, just to make sure we're understanding the same things I'll regress back a bit for terminology sake. When setting an option to multiple(true) you're allowing -O to receive multiple values. (For flags it just allows them to occur multiple times).

What I think is the confusing part is that there are two ways to specify multiple values for an option. The way I believe you're thinking of, where each occurrence of that option is followed by a single (and only a single) value, such as -O val1 -O val2 etc. There is also a shorthand way, because some CLIs expect to be able to pass multiple values without having to re-pass the flag, i.e. these two being equivalent

$ program -o val1 val2
$ program -o val1 -o val2

When you tell clap and option is .multiple(true) the only thing you're telling it is "this thing can have > 1 values." That's it. The important part is what still has not been specified, such as how many values are possible, or what range, or even even how many values per occurrence of the option. So literally the only thing clap knows is keep parsing values for -O until it hits one of these three conditions:

  1. It runs into another flag or special character -f, --, subcommand, etc.
  2. It reaches the upper limit for max values (i.e. when you set .max_values(1))
  3. It reaches a specific number of values per occurrence (i.e. such as would be set by number_of_values(1)).

So you might ask, what's the difference between number_of_values and max_values and multiple?

number_of_values looks for a specific number of values per instance of said option. i.e. you set it to 2 and it will look for -O val1 val2 then quit parsing values for -O, but this also means -O val2 or -O val1 val2 val3 is an error.

max_values on the otherhand just sets an upper bound on the number of values allowed, and again will continue parsing values until one of those three conditions above is met.

Hopefully this is more clear, please let me know if it's not, or where you found the documentation lacking. If everything I explained was already understood and you mean something different let me know, cause then it may be a bug. Also, if something I said is instead wrong (logically, not programatically) please let me know and we can find a way to correct it.

Thanks for taking the time to file this!

@kbknapp kbknapp added T: RFC / question A-docs Area: documentation, including docs.rs, readme, examples, etc... labels Feb 4, 2016
@kbknapp
Copy link
Member

kbknapp commented Feb 4, 2016

This did in fact point out a bug! Everything I said above is still true, just there is a current bug not respecting number_of_values 😉

@kbknapp kbknapp added C-bug Category: Updating dependencies P1: urgent A-parsing Area: Parser's logic and needs it changed somehow. and removed P2: need to have labels Feb 4, 2016
kbknapp added a commit that referenced this issue Feb 4, 2016
i.e. assume, option -O set to multiple, and number_of_values(1)
set. And assume a *required* positional argument is also set.

-O some -O other pos

Would fail with pos arg not supplied.

Relates to #415
@kbknapp kbknapp mentioned this issue Feb 4, 2016
@kbknapp
Copy link
Member

kbknapp commented Feb 4, 2016

#417 fixes the issue I was talking about...once that merges everything I said above will be for real 😉

@birkenfeld
Copy link
Contributor Author

Ah, so I should have been using number_of_values and not max_values. Makes sense - although I still think that it should be 1 by default - but as long as I can set this I'm happy.

@kbknapp
Copy link
Member

kbknapp commented Feb 5, 2016

Yeah looking back, this is one other slightly breaking change I wish I'd made during the bump to 2.x because I agree the largest source of confusion is that it's not 1 by default (well except those that expect that). But going the other way around (opt in to > 1) has less potential for confusion.

@kbknapp
Copy link
Member

kbknapp commented Feb 5, 2016

Also #417 is about to merge, so I'll put out 2.0.5 right after that.

homu added a commit that referenced this issue Feb 5, 2016
homu added a commit that referenced this issue Feb 5, 2016
@kbknapp
Copy link
Member

kbknapp commented Feb 5, 2016

2.0.5 is out on crates.io. I'm going to leave this issue open until I re-do the docs for these parts.

@kbknapp kbknapp added P2: need to have and removed C: args A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies labels Feb 5, 2016
@kbknapp kbknapp closed this as completed Feb 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation, including docs.rs, readme, examples, etc...
Projects
None yet
Development

No branches or pull requests

2 participants