From bdf205bff25c41e7407822d93e6bac71672f0448 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 21 Jul 2023 16:05:38 -0500 Subject: [PATCH 1/2] test(parser): Show one value terminator bug --- tests/builder/multiple_values.rs | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index 421d61924ee..5999fba6729 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -1375,6 +1375,39 @@ fn multiple_value_terminator_option_other_arg() { assert!(*m.get_one::("flag").expect("defaulted by clap")); } +#[test] +fn all_multiple_value_terminator() { + let m = Command::new("lip") + .arg( + Arg::new("files") + .value_terminator(";") + .action(ArgAction::Set) + .num_args(0..), + ) + .arg(Arg::new("other").num_args(0..)) + .try_get_matches_from(vec!["test", "value", ";"]); + + assert!(m.is_ok(), "{:?}", m.unwrap_err().kind()); + let m = m.unwrap(); + + assert!(m.contains_id("files")); + assert!(m.contains_id("other")); + assert_eq!( + m.get_many::("files") + .unwrap() + .map(|v| v.as_str()) + .collect::>(), + ["value".to_owned()], + ); + assert_eq!( + m.get_many::("other") + .unwrap() + .map(|v| v.as_str()) + .collect::>(), + [";".to_owned()], + ); +} + #[test] fn multiple_vals_with_hyphen() { let res = Command::new("do") From 8bee7280342d85abb6f2b0913a02075c8f0045e5 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 21 Jul 2023 16:41:08 -0500 Subject: [PATCH 2/2] fix(parser): Value terminator has higher precedence than later multiple values This is one of several bugs found when looking at #4960. --- clap_builder/src/parser/parser.rs | 9 ++++++++- tests/builder/multiple_values.rs | 9 +-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/clap_builder/src/parser/parser.rs b/clap_builder/src/parser/parser.rs index a4e15fef49d..d6d8e8da436 100644 --- a/clap_builder/src/parser/parser.rs +++ b/clap_builder/src/parser/parser.rs @@ -302,6 +302,13 @@ impl<'cmd> Parser<'cmd> { .map(|p_name| !p_name.is_last_set()) .unwrap_or_default(); + let is_terminated = self + .cmd + .get_keymap() + .get(&pos_counter) + .map(|a| a.get_value_terminator().is_some()) + .unwrap_or_default(); + let missing_pos = self.cmd.is_allow_missing_positional_set() && is_second_to_last && !trailing_values; @@ -309,7 +316,7 @@ impl<'cmd> Parser<'cmd> { debug!("Parser::get_matches_with: Positional counter...{pos_counter}"); debug!("Parser::get_matches_with: Low index multiples...{low_index_mults:?}"); - if low_index_mults || missing_pos { + if (low_index_mults || missing_pos) && !is_terminated { let skip_current = if let Some(n) = raw_args.peek(&args_cursor) { if let Some(arg) = self .cmd diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index 5999fba6729..57691f177f1 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -1391,7 +1391,7 @@ fn all_multiple_value_terminator() { let m = m.unwrap(); assert!(m.contains_id("files")); - assert!(m.contains_id("other")); + assert!(!m.contains_id("other")); assert_eq!( m.get_many::("files") .unwrap() @@ -1399,13 +1399,6 @@ fn all_multiple_value_terminator() { .collect::>(), ["value".to_owned()], ); - assert_eq!( - m.get_many::("other") - .unwrap() - .map(|v| v.as_str()) - .collect::>(), - [";".to_owned()], - ); } #[test]