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

Using index_of()/indices_of() with optional arguments that take optional values #2419

Open
2 tasks done
cptpcrd opened this issue Mar 20, 2021 · 8 comments
Open
2 tasks done
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing

Comments

@cptpcrd
Copy link

cptpcrd commented Mar 20, 2021

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Describe your use case

I have multiple optional arguments with optional values. If several of them were specified without passing a value, I want to determine the indices of the arguments so I can find out which one was specified first. However, it seems that this is one of the few cases where Arg::index_of()/Arg::indices_of() doesn't work.

A simple example can be found below.

Describe the solution you'd like

Ideally, Arg::index_of() would work for arguments with optional values.

Alternatives, if applicable

Perhaps a new method, say Args::index_of_opt() (which would return the index of the flag like -a or --all`, instead of the value) could be added?

Additional Context

I'm using structopt, but I've translated this to clap code.

Example with clap 2.33:

fn main() {
    use clap::{Arg, App};
    
    let app = App::new("Basic Test")
        .arg(Arg::with_name("abc")
            .short("a")
            .long("abc")
            .takes_value(true)
            .multiple(false)
            .min_values(0)
            .max_values(1));

    // I can get the index in all of these cases (because clap returns the index of the value)

    let matches = app.clone().get_matches_from(&["", "-a1"]);
    println!("{:?}", matches.index_of("abc"));

    let matches = app.clone().get_matches_from(&["", "--abc=1"]);
    println!("{:?}", matches.index_of("abc"));

    let matches = app.clone().get_matches_from(&["", "-a", "1"]);
    println!("{:?}", matches.index_of("abc"));

    let matches = app.clone().get_matches_from(&["", "--abc", "1"]);
    println!("{:?}", matches.index_of("abc"));

    // But I can't get the index in these cases (which is when I really need it!)

    let matches = app.clone().get_matches_from(&["", "-a"]);
    println!("{:?}", matches.index_of("abc"));

    let matches = app.clone().get_matches_from(&["", "--abc"]);
    println!("{:?}", matches.index_of("abc"));
}

This produces the following output:

Some(2)
Some(2)
Some(2)
Some(2)
None
None
Updated for clap 3 (produces the same output as the above version when run against the latest master):
fn main() {
    use clap::{Arg, App};
    
    let app = App::new("Basic Test")
        .arg(Arg::new("abc")
            .short('a')
            .long("abc")
            .takes_value(true)
            .multiple(false)
            .min_values(0)
            .max_values(1));

    // I can get the index in all of these cases (because clap returns the index of the value)

    let matches = app.clone().get_matches_from(&["", "-a1"]);
    println!("{:?}", matches.index_of("abc"));

    let matches = app.clone().get_matches_from(&["", "--abc=1"]);
    println!("{:?}", matches.index_of("abc"));

    let matches = app.clone().get_matches_from(&["", "-a", "1"]);
    println!("{:?}", matches.index_of("abc"));

    let matches = app.clone().get_matches_from(&["", "--abc", "1"]);
    println!("{:?}", matches.index_of("abc"));

    // But I can't get the index in these cases (which is when I really need it!)

    let matches = app.clone().get_matches_from(&["", "-a"]);
    println!("{:?}", matches.index_of("abc"));

    let matches = app.clone().get_matches_from(&["", "--abc"]);
    println!("{:?}", matches.index_of("abc"));
}
@ldm0 ldm0 added this to the 3.0 milestone Mar 20, 2021
@pksunkara pksunkara modified the milestones: 3.0, 3.1 Mar 20, 2021
@epage epage added A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations and removed C: matches labels Dec 8, 2021
@epage
Copy link
Member

epage commented Dec 13, 2021

Currently, we only support indices for values, not arguments. This will require some design work for what the API / behavior should be.

If several of them were specified without passing a value, I want to determine the indices of the arguments so I can find out which one was specified first.

Could you elaborate on your use case for why the order they are set matters? Its helpful to have the wider context on these types of things.

I'm using structopt, but I've translated this to clap code.

How are you getting the index with structopt?

@epage epage removed this from the 3.1 milestone Dec 13, 2021
@epage epage added the S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing label Dec 13, 2021
@cptpcrd
Copy link
Author

cptpcrd commented Dec 13, 2021

Could you elaborate on your use case for why the order they are set matters? Its helpful to have the wider context on these types of things.

I'm trying to match the behavior of a C program where the order matters. That isn't terribly uncommon for C programs; e.g. tar -C.

How are you getting the index with structopt?

Something like this (see docs):

use structopt::StructOpt;

#[derive(Debug, StructOpt)]
struct Opt {
    #[structopt(short, long)]
    debug: bool,
}

fn main() {
    let matches = Opt::clap().get_matches_from(&["", "-d"]);
    let opt = Opt::from_clap(&matches);
    println!("{:?}", opt);
    println!("{:?}", matches.index_of("debug"));
}

@fabianfreyer
Copy link

fabianfreyer commented May 15, 2023

I'm running into a related issue (#4908). In my use case there are multiple configuration layers that override each other (e.g. --config-file /path/to/file --env --url https://example.com/, where ordering of each option matters. One of these options takes an optional argument. In this case, index_of doesn't return the index of the argument.

@epage:

Currently, we only support indices for values, not arguments. This will require some design work for what the API / behavior should be.

Is this still true? The docs say:

For flags (i.e. those arguments which don’t have an associated value), indices refer to occurrence of the switch, such as -f, or --flag. However, for options the indices refer to the values -o val would therefore not represent two distinct indices, only the index for val would be recorded. This is by design.

FWIW, this would be the behaviour I'd expect:

use clap;

fn main() {
    use clap::{Arg, Command};
    
    let app = Command::new("Basic Test")
        .arg(Arg::new("optional")
            .short('o')
            .num_args(0..=1))
        .arg(Arg::new("flag")
            .short('f')
            .num_args(0))
        .arg(Arg::new("value")
            .short('v')
            .num_args(1));

    let matches = app.clone().get_matches_from(&["", "-f"]);
    assert_eq!(matches.index_of("flag"), Some(1));

    let matches = app.clone().get_matches_from(&["", "-v", "42"]);
    assert_eq!(matches.index_of("value"), Some(2));

    let matches = app.clone().get_matches_from(&["", "-o", "42"]);
    assert_eq!(matches.index_of("optional"), Some(2));
    
    let matches = app.clone().get_matches_from(&["", "-o"]);
    assert_eq!(matches.index_of("optional"), Some(1)); // Fails, returns None.
}

I've drafted up PR #4909 to implement this.

fabianfreyer added a commit to fabianfreyer/clap that referenced this issue May 15, 2023
When an argument takes an optional value (e.g. `.num_args(0..=1)`), make sure an index is pushed:
* when a value is specified, for each value
* when no value is specified, for the option, similar to a flag.

Note: this means that a MatchedArg's `num_vals` no longer necessarily matches the number of indices.

Fixes clap-rs#2419
fabianfreyer added a commit to fabianfreyer/clap that referenced this issue May 15, 2023
When an argument takes an optional value (e.g. `.num_args(0..=1)`), make
sure an index is pushed:
* when a value is specified, for each value
* when no value is specified, for the option, similar to a flag.

Note: this means that a MatchedArg's `num_vals` no longer necessarily
matches the number of indices.

Fixes clap-rs#2419
@fabianfreyer
Copy link

Perhaps opening a PR was the wrong thing to do here -- what would be the best way to help move this forward?

@epage
Copy link
Member

epage commented May 17, 2023

Is this still true? The docs say:

Nuance: it is true for ArgAction::Set and ArgAction::Append. It is not true for ArgAction::SetTrue.

When you do num_args(0..=1), that is still an ArgAction::Set even if it still sometimes acts like ArgAction::SetTrue.

FWIW, this would be the behaviour I'd expect:

That works for you, but what about others? Users are relying on a mapping between values and indices.

Perhaps opening a PR was the wrong thing to do here -- what would be the best way to help move this forward?

Examine the relevant use cases for indices and come up with a proposal that meets them. If its a breaking change, evaluate if there is a way to opt into it now or whether we have to wait on clap v5.0.0 for this.

@fabianfreyer
Copy link

Right, so, use cases. There seem to be two disjunct use cases here:

  1. Keeping track of the order of values: this is currently supported, and there is a 1:1 mapping between an ArgMatch's values (e.g. via ArgMatches::get_many), which do not include any occurrences of an Arg with no value.
    --arg --arg foo --arg bar
    
    Given the above arguments, the expected behavior would be:
    values = ["foo", "bar"]
    indices = [1, 2]
  2. Keeping track of an Arg's occurrences, independently of whether any values are passed: this is what was proposed by Push indices for arguments taking optional values #4909. In this case, there would need to be a 1:1 mapping between the list of indices and the list of occurrences, e.g.:
    --arg --arg foo --arg bar
    
    Given the above arguments, the expected behavior would be:
    occurrences = [[], ["foo"], ["bar"]]
    indices = [1, 2, 3]

I currently see the following ways forward:

  1. Introduce a separate index, e.g. ArgMatches::ocurrence_indices(). Pro: This would allow opt-in to this functionality without breaking changes. Contra: It's confusing and clutters ArgMatches.
  2. Implement functionality as proposed in Push indices for arguments taking optional values #4909. This would be a breaking change. An argument can be made that users who allow optional arguments should use occurrences to handle the cases they explicitely allow. This could be gated behind a feature flag which is removed in clap v5.0.
    Pro: The code for this is already around, and it's trivial to adapt.
    Contra: Asking users to use ocurrences to iterate over optional values may be confusing.
  3. Enforce downcasting in ArgMatches::get_many and similar functions for optional argument values to types that support emptiness, e.g. Option (num_args(0..=1)) and/or iterable types such as Vec (num_args(0..n)). This would be a breaking change, which could be gated behind a feature flag which is removed in clap v5.0.
    Pro: indices would map directly to ocurrences and values, and values for optional arguments are inherently optional (Option or empty Vecs etc), leading to more idiomatic code.
    Contra: Arguably the most breaking change.

A mixture of these approaches could also be possible, e.g. going with 1.) for clap v4 and implementing 3.) for clap v5 and feature-gating it for clap v4, then deprecating 1.) in clap v5.

I'd be curious to see what aspects I missed, because I'm sure there are some.

@epage
Copy link
Member

epage commented Jun 14, 2023

For some extra context, we did not expose grouping-by-occurrence until clap v4.1.0 (see #2924), so index per occurrence was not possible until then.

An extra challenge in all of this is #3846. Would occurrence indexes (whether replacing value indexes or parallel to it) make that issue harder, easier, or no difference?

@fabianfreyer
Copy link

TL;DR:
Ocurrence indices don't make #3846 harder. They do make it easier if vectors of optional values are considered. Otherwise, there is no difference.

Long form:
I think that would depend on details. This issue is about having a mapping of indices to ocurrences, whereas #3846 is about having a mapping of values to their source locations (and hence indices). While not every value can be assigned an index (e.g. what index would a default value have?), if we constrain that to values set by arguments, then every value should be attributable to a specific occurrence of an argument, and hence, an index.

Current value indices form a subset of the proposed occurrence indices, every value is part of an ocurrence, but not all ocurrences have values:

                --arg --arg foo --arg bar
value idx:                  ^0        ^1
occurrence idx:      ^0     ^1        ^2

Therefore, using ocurrence indices instead of value indices definitely don't make it harder to find an index for every value (i.e. #3846).

Now consider (since I'm not sure how to do num_args via derive) lists of optional values, e,g.:

#[derive(Args)]
struct MyArgs {
    // --arg1 --arg1=42 -> vec![None, Some(42)]
    #[clap(short, long, value_source)]
    pub arg1: Vec<Value<Option<u8>>>,

    // --arg2 --arg2 foo --arg2 foo bar -> vec![vec![], vec!["foo"], vec!["foo", "bar"]]
    #[clap(short, long, value_source)]
    pub arg1: Vec<Value<Vec<String>>>,
}

In these cases, finding an index to assign to the empty values would be possible using occurrence indices, but not possible using value indices. (i.e. making #3864 easier) However, since args taking a variable number of values seem to not be supported like that using derive (unless I'm mistaken?) at the moment, I'd say at the moment it doesn't make a difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants