Repository: clap-rs/clap
Pull request #2048 (2020-08-05 14:28:29)

Change the ret tyte of Argmatches::subcommand
<!--
If your PR closes some issues, please write `Closes #XXXX`
where `XXXX` is the number of the issue you want to fix.
Each issue goes on its own line.
-->
The question was raised in https://github.com/clap-rs/clap/discussions/2047. And indeed, having both the name and the matches wrapped in `Option` feels more fitting.


Patch:
diff --git a/clap_derive/src/derives/from_arg_matches.rs b/clap_derive/src/derives/from_arg_matches.rs
index 195d2fe4..d6348ca4 100644
--- a/clap_derive/src/derives/from_arg_matches.rs
+++ b/clap_derive/src/derives/from_arg_matches.rs
@@ -65,9 +65,7 @@ pub fn gen_for_enum(name: &Ident) -> TokenStream {
         #[deny(clippy::correctness)]
         impl ::clap::FromArgMatches for #name {
             fn from_arg_matches(matches: &::clap::ArgMatches) -> Self {
-                let (name, subcmd) = matches.subcommand();
-                <#name as ::clap::Subcommand>::from_subcommand(name, subcmd)
-                    .unwrap()
+                <#name as ::clap::Subcommand>::from_subcommand(matches.subcommand()).unwrap()
             }
         }
     }
@@ -107,11 +105,7 @@ pub fn gen_constructor(fields: &Punctuated<Field, Comma>, parent_attribute: &Att
                 };
                 quote_spanned! { kind.span()=>
                     #field_name: {
-                        let (name, subcmd) = matches.subcommand();
-                        <#subcmd_type as ::clap::Subcommand>::from_subcommand(
-                            name,
-                            subcmd
-                        )
+                        <#subcmd_type as ::clap::Subcommand>::from_subcommand(matches.subcommand())
                         #unwrapper
                     }
                 }
diff --git a/clap_derive/src/derives/subcommand.rs b/clap_derive/src/derives/subcommand.rs
index ba4192b2..3bc0f843 100644
--- a/clap_derive/src/derives/subcommand.rs
+++ b/clap_derive/src/derives/subcommand.rs
@@ -238,7 +238,7 @@ fn gen_from_subcommand(
         };
 
         quote! {
-            (#sub_name, Some(matches)) => {
+            Some((#sub_name, matches)) => {
                 Some(#name :: #variant_name #constructor_block)
             }
         }
@@ -249,7 +249,7 @@ fn gen_from_subcommand(
             Unnamed(ref fields) if fields.unnamed.len() == 1 => {
                 let ty = &fields.unnamed[0];
                 quote! {
-                    if let Some(res) = <#ty as ::clap::Subcommand>::from_subcommand(other.0, other.1) {
+                    if let Some(res) = <#ty as ::clap::Subcommand>::from_subcommand(other) {
                         return Some(#name :: #variant_name (res));
                     }
                 }
@@ -263,9 +263,9 @@ fn gen_from_subcommand(
 
     let wildcard = match ext_subcmd {
         Some((span, var_name, str_ty, values_of)) => quote_spanned! { span=>
-            ("", ::std::option::Option::None) => ::std::option::Option::None,
+            None => ::std::option::Option::None,
 
-            (external, Some(matches)) => {
+            Some((external, matches)) => {
                 ::std::option::Option::Some(#name::#var_name(
                     ::std::iter::once(#str_ty::from(external))
                     .chain(
@@ -274,25 +274,14 @@ fn gen_from_subcommand(
                     .collect::<::std::vec::Vec<_>>()
                 ))
             }
-
-            (external, None) => {
-                ::std::option::Option::Some(#name::#var_name({
-                    let mut v = ::std::vec::Vec::with_capacity(1);
-                    v.push(#str_ty::from(external));
-                    v
-                }))
-            }
         },
 
         None => quote!(_ => None),
     };
 
     quote! {
-        fn from_subcommand<'b>(
-            name: &'b str,
-            sub: Option<&'b ::clap::ArgMatches>) -> Option<Self>
-        {
-            match (name, sub) {
+        fn from_subcommand(subcommand: Option<(&str, &::clap::ArgMatches)>) -> Option<Self> {
+            match subcommand {
                 #( #match_arms, )*
                 other => {
                     #( #child_subcommands )else*
diff --git a/clap_derive/src/dummies.rs b/clap_derive/src/dummies.rs
index ef00f663..4d8d2af3 100644
--- a/clap_derive/src/dummies.rs
+++ b/clap_derive/src/dummies.rs
@@ -44,7 +44,7 @@ pub fn from_arg_matches(name: &Ident) {
 pub fn subcommand(name: &Ident) {
     append_dummy(quote! {
         impl ::clap::Subcommand for #name {
-            fn from_subcommand(_name: &str, _matches: Option<&::clap::ArgMatches>) -> Option<Self> {
+            fn from_subcommand(_sub: Option<(&str, &::clap::ArgMatches)>) -> Option<Self> {
                 unimplemented!()
             }
             fn augment_subcommands(_app: ::clap::App<'_>) -> ::clap::App<'_> {
diff --git a/clap_generate/src/generators/mod.rs b/clap_generate/src/generators/mod.rs
index 2dfce022..ef1e080b 100644
--- a/clap_generate/src/generators/mod.rs
+++ b/clap_generate/src/generators/mod.rs
@@ -140,12 +140,19 @@ pub trait Generator {
             .flatten()
             .collect();
 
-        if !shorts_and_visible_aliases.iter().any(|&x| x == 'h') {
+        if shorts_and_visible_aliases
+            .iter()
+            .find(|x| **x == 'h')
+            .is_none()
+        {
             shorts_and_visible_aliases.push('h');
         }
 
         if !p.is_set(AppSettings::DisableVersion)
-            && !shorts_and_visible_aliases.iter().any(|&x| x == 'V')
+            && shorts_and_visible_aliases
+                .iter()
+                .find(|x| **x == 'V')
+                .is_none()
         {
             shorts_and_visible_aliases.push('V');
         }
@@ -169,11 +176,13 @@ pub trait Generator {
             })
             .collect();
 
-        if !longs.iter().any(|x| *x == "help") {
+        if longs.iter().find(|x| **x == "help").is_none() {
             longs.push(String::from("help"));
         }
 
-        if !p.is_set(AppSettings::DisableVersion) && !longs.iter().any(|x| *x == "version") {
+        if !p.is_set(AppSettings::DisableVersion)
+            && longs.iter().find(|x| **x == "version").is_none()
+        {
             longs.push(String::from("version"));
         }
 
@@ -187,7 +196,7 @@ pub trait Generator {
 
         let mut flags: Vec<_> = p.get_flags_no_heading().cloned().collect();
 
-        if !flags.iter().any(|x| x.get_name() == "help") {
+        if flags.iter().find(|x| x.get_name() == "help").is_none() {
             flags.push(
                 Arg::new("help")
                     .short('h')
@@ -197,7 +206,7 @@ pub trait Generator {
         }
 
         if !p.is_set(AppSettings::DisableVersion)
-            && !flags.iter().any(|x| x.get_name() == "version")
+            && flags.iter().find(|x| x.get_name() == "version").is_none()
         {
             flags.push(
                 Arg::new("version")
diff --git a/examples/16_app_settings.rs b/examples/16_app_settings.rs
index a76c6d0e..68c06f32 100644
--- a/examples/16_app_settings.rs
+++ b/examples/16_app_settings.rs
@@ -28,8 +28,8 @@ fn main() {
         println!("The input file is: {}", inp);
     }
 
-    match matches.subcommand() {
-        ("test", _) => println!("The 'test' subcommand was used"),
+    match matches.subcommand_name() {
+        Some("test") => println!("The 'test' subcommand was used"),
         _ => unreachable!(),
     }
 }
diff --git a/examples/20_subcommands.rs b/examples/20_subcommands.rs
index 27320776..c47c891d 100644
--- a/examples/20_subcommands.rs
+++ b/examples/20_subcommands.rs
@@ -120,24 +120,24 @@ fn main() {
     // The most common way to handle subcommands is via a combined approach using
     // `ArgMatches::subcommand` which returns a tuple of both the name and matches
     match matches.subcommand() {
-        ("clone", Some(clone_matches)) => {
+        Some(("clone", clone_matches)) => {
             // Now we have a reference to clone's matches
             println!("Cloning {}", clone_matches.value_of("repo").unwrap());
         }
-        ("push", Some(push_matches)) => {
+        Some(("push", push_matches)) => {
             // Now we have a reference to push's matches
             match push_matches.subcommand() {
-                ("remote", Some(remote_matches)) => {
+                Some(("remote", remote_matches)) => {
                     // Now we have a reference to remote's matches
                     println!("Pushing to {}", remote_matches.value_of("repo").unwrap());
                 }
-                ("local", Some(_)) => {
+                Some(("local", _)) => {
                     println!("'git push local' was used");
                 }
                 _ => unreachable!(),
             }
         }
-        ("add", Some(add_matches)) => {
+        Some(("add", add_matches)) => {
             // Now we have a reference to add's matches
             println!(
                 "Adding {}",
@@ -148,7 +148,7 @@ fn main() {
                     .join(", ")
             );
         }
-        ("", None) => println!("No subcommand was used"), // If no subcommand was usd it'll match the tuple ("", None)
+        None => println!("No subcommand was used"), // If no subcommand was used it'll match the tuple ("", None)
         _ => unreachable!(), // If all subcommands are defined above, anything else is unreachabe!()
     }
 
diff --git a/examples/23_flag_subcommands_pacman.rs b/examples/23_flag_subcommands_pacman.rs
index 77f7e9ab..42aea7f5 100644
--- a/examples/23_flag_subcommands_pacman.rs
+++ b/examples/23_flag_subcommands_pacman.rs
@@ -86,7 +86,7 @@ fn main() {
         .get_matches();
 
     match matches.subcommand() {
-        ("sync", Some(sync_matches)) => {
+        Some(("sync", sync_matches)) => {
             if sync_matches.is_present("search") {
                 let packages: Vec<_> = sync_matches.values_of("search").unwrap().collect();
                 let values = packages.join(", ");
@@ -103,7 +103,7 @@ fn main() {
                 println!("Installing {}...", values);
             }
         }
-        ("query", Some(query_matches)) => {
+        Some(("query", query_matches)) => {
             if let Some(packages) = query_matches.values_of("info") {
                 let comma_sep = packages.collect::<Vec<_>>().join(", ");
                 println!("Retrieving info for {}...", comma_sep);
diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs
index 8681d3a9..311dbece 100644
--- a/src/build/app/mod.rs
+++ b/src/build/app/mod.rs
@@ -2549,7 +2549,8 @@ impl<'help> App<'help> {
     pub(crate) fn has_visible_subcommands(&self) -> bool {
         self.subcommands
             .iter()
-            .any(|sc| sc.name != "help" && !sc.is_set(AppSettings::Hidden))
+            .filter(|sc| sc.name != "help")
+            .any(|sc| !sc.is_set(AppSettings::Hidden))
     }
 
     /// Check if this subcommand can be referred to as `name`. In other words,
diff --git a/src/build/app/settings.rs b/src/build/app/settings.rs
index 736b784a..31eac865 100644
--- a/src/build/app/settings.rs
+++ b/src/build/app/settings.rs
@@ -387,7 +387,7 @@ pub enum AppSettings {
     /// // All trailing arguments will be stored under the subcommand's sub-matches using an empty
     /// // string argument name
     /// match m.subcommand() {
-    ///     (external, Some(ext_m)) => {
+    ///     Some((external, ext_m)) => {
     ///          let ext_args: Vec<&str> = ext_m.values_of("").unwrap().collect();
     ///          assert_eq!(external, "subcmd");
     ///          assert_eq!(ext_args, ["--option", "value", "-fff", "--flag"]);
diff --git a/src/derive.rs b/src/derive.rs
index bbe8fdae..f30c7af0 100644
--- a/src/derive.rs
+++ b/src/derive.rs
@@ -60,7 +60,7 @@ pub trait FromArgMatches: Sized {
 /// @TODO @release @docs
 pub trait Subcommand: Sized {
     /// @TODO @release @docs
-    fn from_subcommand(name: &str, matches: Option<&ArgMatches>) -> Option<Self>;
+    fn from_subcommand(subcommand: Option<(&str, &ArgMatches)>) -> Option<Self>;
     /// @TODO @release @docs
     fn augment_subcommands(app: App<'_>) -> App<'_>;
 }
@@ -118,8 +118,8 @@ impl<T: FromArgMatches> FromArgMatches for Box<T> {
 }
 
 impl<T: Subcommand> Subcommand for Box<T> {
-    fn from_subcommand(name: &str, matches: Option<&ArgMatches>) -> Option<Self> {
-        <T as Subcommand>::from_subcommand(name, matches).map(Box::new)
+    fn from_subcommand(subcommand: Option<(&str, &ArgMatches)>) -> Option<Self> {
+        <T as Subcommand>::from_subcommand(subcommand).map(Box::new)
     }
     fn augment_subcommands(app: App<'_>) -> App<'_> {
         <T as Subcommand>::augment_subcommands(app)
diff --git a/src/output/usage.rs b/src/output/usage.rs
index 137c0e54..6a6b0e52 100644
--- a/src/output/usage.rs
+++ b/src/output/usage.rs
@@ -391,17 +391,28 @@ impl<'help, 'app, 'parser> Usage<'help, 'app, 'parser> {
             .flat_map(|g| self.p.app.unroll_args_in_group(&g.id))
             .collect::<Vec<_>>();
 
-        let pmap = unrolled_reqs
-            .iter()
-            .chain(incls.iter())
-            .filter(|a| self.p.app.get_positionals().any(|p| &&p.id == a))
-            .filter(|&pos| matcher.map_or(true, |m| !m.contains(pos)))
-            .filter_map(|pos| self.p.app.find(pos))
-            .filter(|&pos| incl_last || !pos.is_set(ArgSettings::Last))
-            .filter(|pos| !args_in_groups.contains(&pos.id))
-            .map(|pos| (pos.index.unwrap(), pos))
-            .collect::<BTreeMap<u64, &Arg>>(); // sort by index
-
+        let pmap = if let Some(m) = matcher {
+            unrolled_reqs
+                .iter()
+                .chain(incls.iter())
+                .filter(|a| self.p.app.get_positionals().any(|p| &&p.id == a))
+                .filter(|&pos| !m.contains(pos))
+                .filter_map(|pos| self.p.app.find(pos))
+                .filter(|&pos| incl_last || !pos.is_set(ArgSettings::Last))
+                .filter(|pos| !args_in_groups.contains(&pos.id))
+                .map(|pos| (pos.index.unwrap(), pos))
+                .collect::<BTreeMap<u64, &Arg>>() // sort by index
+        } else {
+            unrolled_reqs
+                .iter()
+                .chain(incls.iter())
+                .filter(|a| self.p.app.get_positionals().any(|p| &&p.id == a))
+                .filter_map(|pos| self.p.app.find(pos))
+                .filter(|&pos| incl_last || !pos.is_set(ArgSettings::Last))
+                .filter(|pos| !args_in_groups.contains(&pos.id))
+                .map(|pos| (pos.index.unwrap(), pos))
+                .collect::<BTreeMap<u64, &Arg>>() // sort by index
+        };
         for p in pmap.values() {
             debug!("Usage::get_required_usage_from:iter:{:?}", p.id);
             if args_in_groups.is_empty() || !args_in_groups.contains(&p.id) {
diff --git a/src/parse/matches/arg_matches.rs b/src/parse/matches/arg_matches.rs
index 0b9e45f6..02849bb0 100644
--- a/src/parse/matches/arg_matches.rs
+++ b/src/parse/matches/arg_matches.rs
@@ -886,9 +886,9 @@ impl ArgMatches {
     ///      .get_matches();
     ///
     /// match app_m.subcommand() {
-    ///     ("clone",  Some(sub_m)) => {}, // clone was used
-    ///     ("push",   Some(sub_m)) => {}, // push was used
-    ///     ("commit", Some(sub_m)) => {}, // commit was used
+    ///     Some(("clone",  sub_m)) => {}, // clone was used
+    ///     Some(("push",   sub_m)) => {}, // push was used
+    ///     Some(("commit", sub_m)) => {}, // commit was used
     ///     _                       => {}, // Either no subcommand or one not tested for...
     /// }
     /// ```
@@ -909,7 +909,7 @@ impl ArgMatches {
     /// // All trailing arguments will be stored under the subcommand's sub-matches using an empty
     /// // string argument name
     /// match app_m.subcommand() {
-    ///     (external, Some(sub_m)) => {
+    ///     Some((external, sub_m)) => {
     ///          let ext_args: Vec<&str> = sub_m.values_of("").unwrap().collect();
     ///          assert_eq!(external, "subcmd");
     ///          assert_eq!(ext_args, ["--option", "value", "-fff", "--flag"]);
@@ -920,10 +920,8 @@ impl ArgMatches {
     /// [`ArgMatches::subcommand_matches`]: ./struct.ArgMatches.html#method.subcommand_matches
     /// [`ArgMatches::subcommand_name`]: ./struct.ArgMatches.html#method.subcommand_name
     #[inline]
-    pub fn subcommand(&self) -> (&str, Option<&ArgMatches>) {
-        self.subcommand
-            .as_ref()
-            .map_or(("", None), |sc| (&sc.name[..], Some(&sc.matches)))
+    pub fn subcommand(&self) -> Option<(&str, &ArgMatches)> {
+        self.subcommand.as_ref().map(|sc| (&*sc.name, &sc.matches))
     }
 }
 
diff --git a/src/parse/matches/matched_arg.rs b/src/parse/matches/matched_arg.rs
index e9286bed..67a3dd78 100644
--- a/src/parse/matches/matched_arg.rs
+++ b/src/parse/matches/matched_arg.rs
@@ -35,6 +35,7 @@ impl MatchedArg {
     pub(crate) fn contains_val(&self, val: &str) -> bool {
         self.vals
             .iter()
-            .any(|v| OsString::as_os_str(v) == OsStr::new(val))
+            .map(OsString::as_os_str)
+            .any(|v| v == OsStr::new(val))
     }
 }
diff --git a/src/parse/parser.rs b/src/parse/parser.rs
index 16cc7056..3dcd75cb 100644
--- a/src/parse/parser.rs
+++ b/src/parse/parser.rs
@@ -626,7 +626,8 @@ impl<'help, 'app> Parser<'help, 'app> {
                 .args
                 .args
                 .iter()
-                .find(|a| a.is_positional() && a.index == Some(pos_counter as u64))
+                .filter(|a| a.is_positional())
+                .find(|p| p.index == Some(pos_counter as u64))
             {
                 if p.is_set(ArgSettings::Last) && !self.is_set(AS::TrailingValues) {
                     return Err(ClapError::unknown_argument(
diff --git a/src/parse/validator.rs b/src/parse/validator.rs
index 33bc3284..c8b86c02 100644
--- a/src/parse/validator.rs
+++ b/src/parse/validator.rs
@@ -213,7 +213,8 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
                 .app
                 .groups
                 .iter()
-                .find(|g| !g.multiple && &g.id == name)
+                .filter(|g| !g.multiple)
+                .find(|g| &g.id == name)
             {
                 let conf_with_self = self
                     .p
@@ -310,7 +311,8 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
                         .app
                         .groups
                         .iter()
-                        .find(|g| !g.multiple && g.id == grp)
+                        .filter(|g| !g.multiple)
+                        .find(|g| g.id == grp)
                     {
                         // for g_arg in self.p.app.unroll_args_in_group(&g.name) {
                         //     if &g_arg != name {
@@ -324,7 +326,8 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
                 .app
                 .groups
                 .iter()
-                .find(|g| !g.multiple && g.id == *name)
+                .filter(|g| !g.multiple)
+                .find(|grp| grp.id == *name)
             {
                 debug!("Validator::gather_conflicts:iter:{:?}:group", name);
                 self.c.insert(g.id.clone());
diff --git a/src/util/argstr.rs b/src/util/argstr.rs
index e98c3c39..c12f141b 100644
--- a/src/util/argstr.rs
+++ b/src/util/argstr.rs
@@ -76,7 +76,15 @@ impl<'a> ArgStr<'a> {
     pub(crate) fn trim_start_n_matches(&self, n: usize, ch: u8) -> ArgStr {
         assert!(ch.is_ascii());
 
-        let i = self.0.iter().take(n).take_while(|c| **c == ch).count();
+        let i = self
+            .0
+            .iter()
+            .take(n)
+            .take_while(|c| **c == ch)
+            .enumerate()
+            .last()
+            .map(|(i, _)| i + 1)
+            .unwrap_or(0);
 
         self.split_at_unchecked(i).1
     }
diff --git a/src/util/graph.rs b/src/util/graph.rs
index c0156332..183b2d9d 100644
--- a/src/util/graph.rs
+++ b/src/util/graph.rs
@@ -30,7 +30,12 @@ where
             self.0.push(Child::new(req));
             idx
         } else {
-            self.0.iter().position(|e| e.id == req).unwrap()
+            self.0
+                .iter()
+                .enumerate()
+                .find(|(_, e)| e.id == req)
+                .map(|(i, _)| i)
+                .unwrap()
         }
     }
 


Test patch:
diff --git a/tests/app_settings.rs b/tests/app_settings.rs
index 55b0179a..66025759 100644
--- a/tests/app_settings.rs
+++ b/tests/app_settings.rs
@@ -735,7 +735,7 @@ fn allow_ext_sc_when_sc_required() {
     assert!(res.is_ok());
 
     match res.unwrap().subcommand() {
-        (name, Some(args)) => {
+        Some((name, args)) => {
             assert_eq!(name, "external-cmd");
             assert_eq!(args.values_of_lossy(""), Some(vec!["foo".to_string()]));
         }
@@ -753,7 +753,7 @@ fn external_subcommand_looks_like_built_in() {
     assert!(res.is_ok());
     let m = res.unwrap();
     match m.subcommand() {
-        (name, Some(args)) => {
+        Some((name, args)) => {
             assert_eq!(name, "install-update");
             assert_eq!(args.values_of_lossy(""), Some(vec!["foo".to_string()]));
         }