From ef1424b21c8f6513afedebf380e1b972473ee54a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 29 Jul 2022 19:23:29 -0500 Subject: [PATCH 1/2] refactor(usage): Clarify required gathering --- src/output/usage.rs | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index 7adaf58c6eb..4d721b76c26 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -389,6 +389,7 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { .flat_map(|g| self.cmd.unroll_args_in_group(&g.id)) .collect::>(); + let mut required_opts = IndexSet::new(); for a in unrolled_reqs .iter() .chain(incls.iter()) @@ -405,9 +406,11 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { { debug!("Usage::get_required_usage_from:iter:{:?}", a); let arg = self.cmd.find(a).expect(INTERNAL_ERROR_MSG).to_string(); - ret_val.insert(arg); + required_opts.insert(arg); } - let mut g_vec: Vec = vec![]; + ret_val.extend(required_opts); + + let mut required_groups = IndexSet::new(); for g in unrolled_reqs .iter() .filter(|n| self.cmd.get_groups().any(|g| g.id == **n)) @@ -425,13 +428,11 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { } let elem = self.cmd.format_group(g); - if !g_vec.contains(&elem) { - g_vec.push(elem); - } + required_groups.insert(elem); } - ret_val.extend(g_vec); + ret_val.extend(required_groups); - let mut pvec = unrolled_reqs + let mut required_positionals = unrolled_reqs .iter() .chain(incls.iter()) .filter(|a| self.cmd.get_positionals().any(|p| &&p.id == a)) @@ -443,13 +444,10 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { .filter(|pos| !args_in_groups.contains(&pos.id)) .map(|pos| (pos.index.unwrap(), pos)) .collect::>(); - pvec.sort_by_key(|(ind, _)| *ind); // sort by index - - for (_, p) in pvec { + required_positionals.sort_by_key(|(ind, _)| *ind); // sort by index + for (_, p) in required_positionals { debug!("Usage::get_required_usage_from:push:{:?}", p.id); - if !args_in_groups.contains(&p.id) { - ret_val.insert(p.to_string()); - } + ret_val.insert(p.to_string()); } debug!("Usage::get_required_usage_from: ret_val={:?}", ret_val); From 4c43b21c3545a3272c1ae8058167af3183b122b7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 29 Jul 2022 19:17:36 -0500 Subject: [PATCH 2/2] fix(parser): Include required argument in message When suggesting required arguments, we wanted to avoid an argument showing up in both a group and by itself but we didn't correctly calculate that, causing no required arguments to show up at times. Now, we all use the same pool of information for doing the calculations. This was the type of cleanup that I expected it to drop our binary size but this added 1k to our .text. Strange. Fixes #4004 --- src/output/usage.rs | 107 ++++++++++++++++++--------------------- tests/builder/require.rs | 23 +++++++++ 2 files changed, 73 insertions(+), 57 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index 4d721b76c26..6f7a2cad445 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -382,72 +382,65 @@ impl<'help, 'cmd> Usage<'help, 'cmd> { unrolled_reqs ); - let args_in_groups = self - .cmd - .get_groups() - .filter(|gn| required.contains(&gn.id)) - .flat_map(|g| self.cmd.unroll_args_in_group(&g.id)) - .collect::>(); - + let mut required_groups_members = IndexSet::new(); let mut required_opts = IndexSet::new(); - for a in unrolled_reqs - .iter() - .chain(incls.iter()) - .filter(|name| !self.cmd.get_positionals().any(|p| &&p.id == name)) - .filter(|name| !self.cmd.get_groups().any(|g| &&g.id == name)) - .filter(|name| !args_in_groups.contains(name)) - .filter(|name| { - !(matcher.is_some() - && matcher - .as_ref() - .unwrap() - .check_explicit(name, ArgPredicate::IsPresent)) - }) - { - debug!("Usage::get_required_usage_from:iter:{:?}", a); - let arg = self.cmd.find(a).expect(INTERNAL_ERROR_MSG).to_string(); - required_opts.insert(arg); - } - ret_val.extend(required_opts); - let mut required_groups = IndexSet::new(); - for g in unrolled_reqs - .iter() - .filter(|n| self.cmd.get_groups().any(|g| g.id == **n)) - { - // don't print requirement for required groups that have an arg. - if let Some(m) = matcher { - let have_group_entry = self - .cmd - .unroll_args_in_group(g) - .iter() - .any(|arg| m.check_explicit(arg, ArgPredicate::IsPresent)); - if have_group_entry { - continue; + let mut required_positionals = Vec::new(); + for req in unrolled_reqs.iter().chain(incls.iter()) { + if let Some(arg) = self.cmd.find(req) { + let is_present = matcher + .map(|m| m.check_explicit(req, ArgPredicate::IsPresent)) + .unwrap_or(false); + debug!( + "Usage::get_required_usage_from:iter:{:?} arg is_present={}", + req, is_present + ); + if !is_present { + if arg.is_positional() { + if incl_last || !arg.is_last_set() { + required_positionals.push((arg.index.unwrap(), arg.to_string())); + } + } else { + required_opts.insert(arg.to_string()); + } + } + } else { + debug_assert!(self.cmd.find_group(req).is_some()); + let group_members = self.cmd.unroll_args_in_group(req); + let is_present = matcher + .map(|m| { + group_members + .iter() + .any(|arg| m.check_explicit(arg, ArgPredicate::IsPresent)) + }) + .unwrap_or(false); + debug!( + "Usage::get_required_usage_from:iter:{:?} group is_present={}", + req, is_present + ); + if !is_present { + let elem = self.cmd.format_group(req); + required_groups.insert(elem); + required_groups_members.extend( + group_members + .iter() + .flat_map(|id| self.cmd.find(id)) + .map(|arg| arg.to_string()), + ); } } - - let elem = self.cmd.format_group(g); - required_groups.insert(elem); } + + required_opts.retain(|arg| !required_groups_members.contains(arg)); + ret_val.extend(required_opts); + ret_val.extend(required_groups); - let mut required_positionals = unrolled_reqs - .iter() - .chain(incls.iter()) - .filter(|a| self.cmd.get_positionals().any(|p| &&p.id == a)) - .filter(|&pos| { - matcher.map_or(true, |m| !m.check_explicit(pos, ArgPredicate::IsPresent)) - }) - .filter_map(|pos| self.cmd.find(pos)) - .filter(|&pos| incl_last || !pos.is_last_set()) - .filter(|pos| !args_in_groups.contains(&pos.id)) - .map(|pos| (pos.index.unwrap(), pos)) - .collect::>(); required_positionals.sort_by_key(|(ind, _)| *ind); // sort by index for (_, p) in required_positionals { - debug!("Usage::get_required_usage_from:push:{:?}", p.id); - ret_val.insert(p.to_string()); + if !required_groups_members.contains(&p) { + ret_val.insert(p); + } } debug!("Usage::get_required_usage_from: ret_val={:?}", ret_val); diff --git a/tests/builder/require.rs b/tests/builder/require.rs index 7b480538b23..f2d832a55e2 100644 --- a/tests/builder/require.rs +++ b/tests/builder/require.rs @@ -1413,3 +1413,26 @@ For more information try --help "; utils::assert_output(cmd, "clap-test aaa -b bbb -c ccc", EXPECTED, true); } + +#[test] +fn required_require_with_group_shows_flag() { + let cmd = Command::new("test") + .arg(arg!(--"require-first").requires("first")) + .arg(arg!(--first).group("either_or_both")) + .arg(arg!(--second).group("either_or_both")) + .group( + ArgGroup::new("either_or_both") + .multiple(true) + .required(true), + ); + const EXPECTED: &str = "\ +error: The following required arguments were not provided: + --first + +USAGE: + test --require-first <--first|--second> + +For more information try --help +"; + utils::assert_output(cmd, "test --require-first --second", EXPECTED, true); +}