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 values and occurrences are not correctly counted / grouped for positionals #3763

Closed
2 tasks done
epage opened this issue May 27, 2022 · 1 comment
Closed
2 tasks done
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies E-hard Call for participation: Experience needed to fix: Hard / a lot M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone

Comments

@epage
Copy link
Member

epage commented May 27, 2022

Please complete the following tasks

Rust Version

rustc 1.57.0 (f1edd0429 2021-11-29)

Clap Version

3.1.13

Minimal reproducible code

#[test]
fn values_per_occurrence_positional() {
    let mut a = Command::new("test").arg(
        Arg::new("pos")
            .number_of_values(2)
            .multiple_occurrences(true),
    );

    let m = a.try_get_matches_from_mut(vec!["myprog", "val1", "val2"]);
    let m = match m {
        Ok(m) => m,
        Err(err) => panic!("{}", err),
    };
    assert_eq!(
        m.get_many::<String>("pos")
            .unwrap()
            .map(|v| v.as_str())
            .collect::<Vec<_>>(),
        ["val1", "val2"]
    );
    assert_eq!(m.occurrences_of("pos"), 1);

    let m = a.try_get_matches_from_mut(vec!["myprog", "val1", "val2", "val3", "val4"]);
    let m = match m {
        Ok(m) => m,
        Err(err) => panic!("{}", err),
    };
    assert_eq!(
        m.get_many::<String>("pos")
            .unwrap()
            .map(|v| v.as_str())
            .collect::<Vec<_>>(),
        ["val1", "val2", "val3", "val4"]
    );
    assert_eq!(m.occurrences_of("pos"), 2); // Fails, we don't recognize this as two occurrences
}

#[test]
fn positional_parser_advances() {
    let m = Command::new("multiple_values")
        .arg(Arg::new("pos1").number_of_values(2))
        .arg(Arg::new("pos2").number_of_values(2))
        .try_get_matches_from(vec!["myprog", "val1", "val2", "val3", "val4"]);

    assert!(m.is_ok(), "{}", m.unwrap_err());
    let m = m.unwrap();

    assert_eq!(m.occurrences_of("pos1"), 1);
    assert_eq!(
        m.get_many::<String>("pos1")
            .unwrap()
            .map(|v| v.as_str())
            .collect::<Vec<_>>(),
        ["val1", "val2"]
    );

    assert_eq!(m.occurrences_of("pos2"), 1);
    assert_eq!(
        m.get_many::<String>("pos2")
            .unwrap()
            .map(|v| v.as_str())
            .collect::<Vec<_>>(),
        ["val3", "val4"]
    );
}

Steps to reproduce the bug with the above code

Inject those tests into clap

Actual Behaviour

The first test will report groups correctly but will also report an occurrence per value, contradicting the documentation. In a change I'm working on, it will instead report one occurrence regardless of how many occurrences there should be

The second test is invalid because we don't stop parsing multiple values once we've hit the cap, so we disallow it in a debug assert

Expected Behaviour

Both pass

Additional Context

No response

Debug Output

No response

@epage epage added C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. E-hard Call for participation: Experience needed to fix: Hard / a lot A-parsing Area: Parser's logic and needs it changed somehow. labels May 27, 2022
@epage epage added this to the 4.0 milestone May 27, 2022
@epage epage changed the title multiple values and occurrences are not correctly counted / grou[ed multiple values and occurrences are not correctly counted / grouped May 27, 2022
@epage epage changed the title multiple values and occurrences are not correctly counted / grouped multiple values and occurrences are not correctly counted / grouped for positionals May 31, 2022
@epage
Copy link
Member Author

epage commented Jun 8, 2022

Closing because of #3772

@epage epage closed this as completed Jun 8, 2022
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-bug Category: Updating dependencies E-hard Call for participation: Experience needed to fix: Hard / a lot M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

No branches or pull requests

1 participant