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

Rant: MultipleValues considered harmful #2031

Closed
intgr opened this issue Jul 21, 2020 · 4 comments
Closed

Rant: MultipleValues considered harmful #2031

intgr opened this issue Jul 21, 2020 · 4 comments
Labels
C-bug Category: Updating dependencies
Milestone

Comments

@intgr
Copy link
Contributor

intgr commented Jul 21, 2020

Sorry, but a rant here... I accept that other people might have a different view on this, but let me make my case.

I've only been scratching the surface of clap, both as CLI application developer and hacking clap itself. And I'm already losing count of the number of times I've been confused by the distinction between MultipleValues and MultipleOccurrences.

As positional arg

I was trying to test a positional argument (with TrailingVarArg in #1793) and figure out, which of the two settings is correct. It turns out, both are needed for it to work correctly. With just one or the other set, it results in surprising and non-useful behavior. So I think either it should be an error to use positional args with one Multiple* flag without the other, or the behavior should be changed to only need one.

Code

    App::new("Alter Ego: run desktop applications under a different local user")
        .setting(AppSettings::TrailingVarArg)
        .arg(
            Arg::new("command")
                .about("Command name and arguments to run (default: user shell)")
                .multiple_*(true) // multiple_values/multiple_occurrences
                .value_hint(ValueHint::CommandWithArguments),

Current actual behavior

  • Using MultipleOccurrences: Found argument 'asd' which wasn't expected, or isn't valid in this context
  • Using MultipleValues: The argument '...' was provided more than once, but cannot be used multiple times

As named arg

As for named options, MultipleValues might serve a purpose, but I think it's confusing for users of clap-based applications as well. I can't name a single common Unix utility from the top of my head that would accept multiple values after some --option switch. And there isn't support for such behavior in shell completion either, which is the topic of my PR #1793.

Indeed I'm not the first person working on clap to be confused by this duality; zsh.rs has this:

        // @TODO @soundness should probably be either multiple occurrences or multiple values and
        // not both
        let multiple = if o.is_set(ArgSettings::MultipleOccurrences)
            || o.is_set(ArgSettings::MultipleValues)

I can't help but feel that the MultipleValues feature is a mistake. Or maybe it is useful in some edge case, but should be discouraged and not exist as a prominent Arg::multiple_values() method, and should not be part of Arg::multiple().

Additional context

Using clap latest master branch, commit dc363d0.

@intgr intgr added the C-bug Category: Updating dependencies label Jul 21, 2020
@pksunkara pksunkara added this to the 3.0 milestone Jul 21, 2020
@pksunkara
Copy link
Member

It's better to have them both for normal args, but for positional args, I guess they both mean the same.

intgr added a commit to intgr/clap that referenced this issue Jul 21, 2020
@CreepySkeleton
Copy link
Contributor

Before I follow with a rant of my own - because of course I will, but I need to gather my thoughts or it will be nasty - @intgr , could you check if multiple_occurences(true).takes_value(true) works for you?

@Ch00k
Copy link

Ch00k commented Oct 21, 2020

multiple_occurences(true).takes_value(true) does not work for me in 3.0.0-beta2: Found argument 'foo' which wasn't expected, or isn't valid in this context

@ldm0
Copy link
Member

ldm0 commented Feb 21, 2021

Recent change of MultipleValues related change fixes this problem.

use clap::*;

fn main() {
    let m = App::new("Alter Ego: run desktop applications under a different local user")
        .setting(AppSettings::TrailingVarArg)
        .arg(
            Arg::new("command")
                .about("Command name and arguments to run (default: user shell)")
                .multiple_values(true) // multiple_values/multiple_occurrences
                .value_hint(ValueHint::CommandWithArguments),
        )
        .get_matches();
    dbg!(m);
}

Code above doesn't work correctly with cargo run -- a b c d ef g on 3.0.0-beta.2, but it works correctly on current master. cc @pksunkara .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies
Projects
None yet
Development

No branches or pull requests

5 participants