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

Make overrides_with working when MultipleValues is enabled. #2696

Merged
merged 1 commit into from Aug 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 30 additions & 24 deletions src/build/arg/mod.rs
Expand Up @@ -1096,10 +1096,11 @@ impl<'help> Arg<'help> {
/// **NOTE:** When an argument is overridden it is essentially as if it never was used, any
/// conflicts, requirements, etc. are evaluated **after** all "overrides" have been removed
///
/// **WARNING:** Positional arguments and options which accept [`Multiple*`] cannot override
/// themselves (or we would never be able to advance to the next positional). If a positional
/// argument or option with one of the [`Multiple*`] settings lists itself as an override, it is
/// simply ignored.
/// **WARNING:** Positional arguments and options which accept
/// [`ArgSettings::MultipleOccurrences`] cannot override themselves (or we
/// would never be able to advance to the next positional). If a positional
/// argument or option with one of the [`ArgSettings::MultipleOccurrences`]
/// settings lists itself as an override, it is simply ignored.
///
/// # Examples
///
Expand Down Expand Up @@ -1136,8 +1137,10 @@ impl<'help> Arg<'help> {
/// assert!(m.is_present("flag"));
/// assert_eq!(m.occurrences_of("flag"), 1);
/// ```
/// Making an arg [`Multiple*`] and override itself is essentially meaningless. Therefore
/// clap ignores an override of self if it's a flag and it already accepts multiple occurrences.
///
/// Making an arg [`ArgSettings::MultipleOccurrences`] and override itself
/// is essentially meaningless. Therefore clap ignores an override of self
/// if it's a flag and it already accepts multiple occurrences.
///
/// ```
/// # use clap::{App, Arg};
Expand All @@ -1147,8 +1150,10 @@ impl<'help> Arg<'help> {
/// assert!(m.is_present("flag"));
/// assert_eq!(m.occurrences_of("flag"), 4);
/// ```
/// Now notice with options (which *do not* set one of the [`Multiple*`]), it's as if only the
/// last occurrence happened.
///
/// Now notice with options (which *do not* set
/// [`ArgSettings::MultipleOccurrences`]), it's as if only the last
/// occurrence happened.
///
/// ```
/// # use clap::{App, Arg};
Expand All @@ -1160,36 +1165,37 @@ impl<'help> Arg<'help> {
/// assert_eq!(m.value_of("opt"), Some("other"));
/// ```
///
/// Just like flags, options with one of the [`Multiple*`] set, will ignore the "override self"
/// setting.
/// This will also work when [`ArgSettings::MultipleValues`] is enabled:
///
/// ```
/// # use clap::{App, Arg};
/// let m = App::new("posix")
/// .arg(Arg::from("--opt [val]... 'some option'")
/// .overrides_with("opt"))
/// .get_matches_from(vec!["", "--opt", "first", "over", "--opt", "other", "val"]);
/// .arg(
/// Arg::new("opt")
/// .long("opt")
/// .takes_value(true)
/// .multiple_values(true)
/// .overrides_with("opt")
/// )
/// .get_matches_from(vec!["", "--opt", "1", "2", "--opt", "3", "4", "5"]);
/// assert!(m.is_present("opt"));
/// assert_eq!(m.occurrences_of("opt"), 2);
/// assert_eq!(m.values_of("opt").unwrap().collect::<Vec<_>>(), &["first", "over", "other", "val"]);
/// assert_eq!(m.occurrences_of("opt"), 1);
/// assert_eq!(m.values_of("opt").unwrap().collect::<Vec<_>>(), &["3", "4", "5"]);
/// ```
///
/// A safe thing to do if you'd like to support an option which supports multiple values, but
/// also is "overridable" by itself, is to not use [`UseValueDelimiter`] and *not* use
/// `MultipleValues` while telling users to separate values with a comma (i.e. `val1,val2`)
/// Just like flags, options with [`ArgSettings::MultipleOccurrences`] set
/// will ignore the "override self" setting.
///
/// ```
/// # use clap::{App, Arg};
/// let m = App::new("posix")
/// .arg(Arg::from("--opt [val] 'some option'")
/// .arg(Arg::from("--opt [val]... 'some option'")
/// .overrides_with("opt"))
/// .get_matches_from(vec!["", "--opt=some,other", "--opt=one,two"]);
/// .get_matches_from(vec!["", "--opt", "first", "over", "--opt", "other", "val"]);
/// assert!(m.is_present("opt"));
/// assert_eq!(m.occurrences_of("opt"), 1);
/// assert_eq!(m.values_of("opt").unwrap().collect::<Vec<_>>(), &["one,two"]);
/// assert_eq!(m.occurrences_of("opt"), 2);
/// assert_eq!(m.values_of("opt").unwrap().collect::<Vec<_>>(), &["first", "over", "other", "val"]);
/// ```
/// [`Multiple*`]: crate::ArgSettings::MultipleValues
/// [`UseValueDelimiter`]: ArgSettings::UseValueDelimiter
pub fn overrides_with<T: Key>(mut self, arg_id: T) -> Self {
self.overrides.push(arg_id.into());
self
Expand Down
4 changes: 1 addition & 3 deletions src/parse/parser.rs
Expand Up @@ -1608,10 +1608,8 @@ impl<'help, 'app> Parser<'help, 'app> {
}
}
// Only do self override for argument that is not positional
// argument or flag with one of the Multiple* setting
// enabled(which is a feature).
// argument or flag with MultipleOccurrences setting enabled.
if (self.is_set(AS::AllArgsOverrideSelf) || override_self)
&& !overrider.is_set(ArgSettings::MultipleValues)
&& !overrider.is_set(ArgSettings::MultipleOccurrences)
&& !overrider.is_positional()
{
Expand Down
22 changes: 22 additions & 0 deletions tests/posix_compatible.rs
Expand Up @@ -306,3 +306,25 @@ fn require_overridden_4() {
let err = result.err().unwrap();
assert_eq!(err.kind, ErrorKind::MissingRequiredArgument);
}

#[test]
fn issue_1374_overrides_self_with_multiple_values() {
let app = App::new("test").arg(
Arg::new("input")
.long("input")
.takes_value(true)
.overrides_with("input")
.min_values(0),
);
let m = app
.clone()
.get_matches_from(&["test", "--input", "a", "b", "c", "--input", "d"]);
assert_eq!(m.values_of("input").unwrap().collect::<Vec<_>>(), &["d"]);
let m = app
.clone()
.get_matches_from(&["test", "--input", "a", "b", "--input", "c", "d"]);
assert_eq!(
m.values_of("input").unwrap().collect::<Vec<_>>(),
&["c", "d"]
);
}