Repository: clap-rs/clap
Pull request #1520 (2019-07-17 00:48:10)

style(help): add newline after writing help
Rebase for #1510 


Patch:
diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs
index 690cccb4..b6fc4cfe 100644
--- a/src/build/arg/mod.rs
+++ b/src/build/arg/mod.rs
@@ -200,7 +200,6 @@ impl<'help> Arg<'help> {
                 "value_name" => yaml_to_str!(a, v, value_name),
                 "use_delimiter" => yaml_to_bool!(a, v, use_delimiter),
                 "allow_hyphen_values" => yaml_to_bool!(a, v, allow_hyphen_values),
-                "require_equals" => yaml_to_bool!(a, v, require_equals),
                 "require_delimiter" => yaml_to_bool!(a, v, require_delimiter),
                 "value_delimiter" => yaml_to_str!(a, v, value_delimiter),
                 "required_unless" => yaml_to_str!(a, v, required_unless),
diff --git a/src/mkeymap.rs b/src/mkeymap.rs
index 2e7455c0..f1ef72da 100644
--- a/src/mkeymap.rs
+++ b/src/mkeymap.rs
@@ -292,7 +292,7 @@ mod tests {
     }
 
     #[test]
-    // #[should_panic(expected = "Len changed")]
+    #[should_panic]
     fn insert_duplicate_value() {
         let mut map: MKeyMap = MKeyMap::new();
 
@@ -302,11 +302,11 @@ mod tests {
 
         map.insert(Long(OsString::from("Two")), Arg::with_name("Value1"));
 
-        assert_eq!(map.args.len(), orig_len + 1/* , "Len changed" */);
-        // assert_eq!(
-        //     map.get(&Long(OsString::from("One"))),
-        //     map.get(&Long(OsString::from("Two")))
-        // );
+        assert_eq!(map.args.len(), orig_len);
+        assert_eq!(
+            map.get(&Long(OsString::from("One"))),
+            map.get(&Long(OsString::from("Two")))
+        );
     }
 
     //    #[test]
diff --git a/src/output/help.rs b/src/output/help.rs
index 0a6b533b..221aa4ca 100644
--- a/src/output/help.rs
+++ b/src/output/help.rs
@@ -74,10 +74,10 @@ impl<'b, 'c, 'd, 'w> Help<'b, 'c, 'd, 'w> {
             next_line_help: nlh,
             hide_pv,
             term_w,
-            color,
+            color: color,
             longest: 0,
             force_next_line: false,
-            cizer,
+            cizer: cizer,
             use_long,
         }
     }
@@ -92,6 +92,9 @@ impl<'b, 'c, 'd, 'w> Help<'b, 'c, 'd, 'w> {
         } else {
             self.write_default_help()?;
         }
+
+        write!(self.writer, "\n")?;
+
         Ok(())
     }
 }
diff --git a/src/parse/errors.rs b/src/parse/errors.rs
index 45ce5708..8a7b94d4 100644
--- a/src/parse/errors.rs
+++ b/src/parse/errors.rs
@@ -842,7 +842,7 @@ impl Error {
     }
 
     #[doc(hidden)]
-    pub fn unknown_argument<A, U>(arg: A, did_you_mean: Option<String>, usage: U, color: ColorWhen) -> Self
+    pub fn unknown_argument<A, U>(arg: A, did_you_mean: &str, usage: U, color: ColorWhen) -> Self
     where
         A: Into<String>,
         U: Display,
@@ -852,25 +852,19 @@ impl Error {
             use_stderr: true,
             when: color,
         });
-
-        let suggest_pattern = format!("If you tried to supply `{}` as a PATTERN use `-- {}`", a, a);
-
-        let did_you_mean_message = match did_you_mean {
-            Some(s) => format!("{}\n", s),
-            _ => "\n".to_owned()
-        };
-
         Error {
             message: format!(
                 "{} Found argument '{}' which wasn't expected, or isn't valid in \
-                 this context{}\
-                 {}\n\n\
+                 this context{}\n\
                  {}\n\n\
                  For more information try {}",
                 c.error("error:"),
                 c.warning(&*a),
-                did_you_mean_message,
-                suggest_pattern,
+                if did_you_mean.is_empty() {
+                    "\n".to_owned()
+                } else {
+                    format!("{}\n", did_you_mean)
+                },
                 usage,
                 c.good("--help")
             ),
diff --git a/src/parse/features/suggestions.rs b/src/parse/features/suggestions.rs
index 73fb0420..a7b38ce2 100644
--- a/src/parse/features/suggestions.rs
+++ b/src/parse/features/suggestions.rs
@@ -25,8 +25,10 @@ where
             candidate = Some((confidence, pv.as_ref().to_owned()));
         }
     }
-
-    candidate.map(|(_, candidate)| candidate)
+    match candidate {
+        None => None,
+        Some((_, candidate)) => Some(candidate.to_owned()),
+    }
 }
 
 #[cfg(not(feature = "suggestions"))]
diff --git a/src/parse/matches/arg_matches.rs b/src/parse/matches/arg_matches.rs
index 26c1c237..7821bb60 100644
--- a/src/parse/matches/arg_matches.rs
+++ b/src/parse/matches/arg_matches.rs
@@ -213,14 +213,14 @@ impl ArgMatches {
     /// [`Values`]: ./struct.Values.html
     /// [`Iterator`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html
     pub fn values_of<T: Key>(&self, id: T) -> Option<Values<'_>> {
-        self.args.get(&id.key()).map(|arg| {
+        if let Some(arg) = self.args.get(&id.key()) {
             fn to_str_slice(o: &OsString) -> &str { o.to_str().expect(INVALID_UTF8) }
             let to_str_slice: fn(&OsString) -> &str = to_str_slice; // coerce to fn pointer
-
-            Values {
+            return Some(Values {
                 iter: arg.vals.iter().map(to_str_slice),
-            }
-        })
+            });
+        }
+        None
     }
 
     /// Gets the lossy values of a specific argument. If the option wasn't present at runtime
@@ -248,12 +248,15 @@ impl ArgMatches {
     /// assert_eq!(itr.next(), None);
     /// ```
     pub fn values_of_lossy<T: Key>(&self, id: T) -> Option<Vec<String>> {
-        self.args.get(&id.key()).map(|arg| {
-            arg.vals
-                .iter()
-                .map(|v| v.to_string_lossy().into_owned())
-                .collect()
-        })
+        if let Some(arg) = self.args.get(&id.key()) {
+            return Some(
+                arg.vals
+                    .iter()
+                    .map(|v| v.to_string_lossy().into_owned())
+                    .collect(),
+            );
+        }
+        None
     }
 
     /// Gets a [`OsValues`] struct which is implements [`Iterator`] for [`OsString`] values of a
@@ -290,10 +293,12 @@ impl ArgMatches {
     pub fn values_of_os<'a, T: Key>(&'a self, id: T) -> Option<OsValues<'a>> {
         fn to_str_slice(o: &OsString) -> &OsStr { &*o }
         let to_str_slice: fn(&'a OsString) -> &'a OsStr = to_str_slice; // coerce to fn pointer
-
-        self.args.get(&id.key()).map(|arg| OsValues {
-            iter: arg.vals.iter().map(to_str_slice),
-        })
+        if let Some(arg) = self.args.get(&id.key()) {
+            return Some(OsValues {
+                iter: arg.vals.iter().map(to_str_slice),
+            });
+        }
+        None
     }
 
     /// Returns `true` if an argument was present at runtime, otherwise `false`.
@@ -579,9 +584,12 @@ impl ArgMatches {
     /// [`ArgMatches::index_of`]: ./struct.ArgMatches.html#method.index_of
     /// [delimiter]: ./struct.Arg.html#method.value_delimiter
     pub fn indices_of<T: Key>(&self, id: T) -> Option<Indices<'_>> {
-        self.args.get(&id.key()).map(|arg| Indices {
-            iter: arg.indices.iter().cloned(),
-        })
+        if let Some(arg) = self.args.get(&id.key()) {
+            return Some(Indices {
+                iter: arg.indices.iter().cloned(),
+            });
+        }
+        None
     }
 
     /// Because [`Subcommand`]s are essentially "sub-[`App`]s" they have their own [`ArgMatches`]
diff --git a/src/parse/parser.rs b/src/parse/parser.rs
index 64072b32..314fe73a 100644
--- a/src/parse/parser.rs
+++ b/src/parse/parser.rs
@@ -459,7 +459,7 @@ where
                                 {
                                     return Err(ClapError::unknown_argument(
                                         &*arg_os.to_string_lossy(),
-                                        None,
+                                        "",
                                         &*Usage::new(self).create_usage_with_title(&[]),
                                         self.app.color(),
                                     ));
@@ -578,7 +578,7 @@ where
                 if p.is_set(ArgSettings::Last) && !self.is_set(AS::TrailingValues) {
                     return Err(ClapError::unknown_argument(
                         &*arg_os.to_string_lossy(),
-                        None,
+                        "",
                         &*Usage::new(self).create_usage_with_title(&[]),
                         self.app.color(),
                     ));
@@ -652,7 +652,7 @@ where
             {
                 return Err(ClapError::unknown_argument(
                     &*arg_os.to_string_lossy(),
-                    None,
+                    "",
                     &*Usage::new(self).create_usage_with_title(&[]),
                     self.app.color(),
                 ));
@@ -677,7 +677,7 @@ where
             } else {
                 return Err(ClapError::unknown_argument(
                     &*arg_os.to_string_lossy(),
-                    None,
+                    "",
                     &*Usage::new(self).create_usage_with_title(&[]),
                     self.app.color(),
                 ));
@@ -1129,7 +1129,7 @@ where
                 let arg = format!("-{}", c);
                 return Err(ClapError::unknown_argument(
                     &*arg,
-                    None,
+                    "",
                     &*Usage::new(self).create_usage_with_title(&[]),
                     self.app.color(),
                 ));
@@ -1499,16 +1499,9 @@ where
             })
             .cloned()
             .collect();
-
-        let did_you_mean_msg = if suffix.0.is_empty() {
-            None
-        } else {
-            Some(suffix.0)
-        };
-
         Err(ClapError::unknown_argument(
             &*format!("--{}", arg),
-            did_you_mean_msg,
+            &*suffix.0,
             &*Usage::new(self).create_usage_with_title(&*used),
             self.app.color(),
         ))
diff --git a/src/util/strext.rs b/src/util/strext.rs
index 5886d6cb..6f81367a 100644
--- a/src/util/strext.rs
+++ b/src/util/strext.rs
@@ -8,9 +8,9 @@ impl _StrExt for str {
         if index == self.len() {
             return true;
         }
-
-        self.as_bytes()
-            .get(index)
-            .map_or(false, |&b| b < 128 || b >= 192)
+        match self.as_bytes().get(index) {
+            None => false,
+            Some(&b) => b < 128 || b >= 192,
+        }
     }
 }


Test patch:
diff --git a/tests/groups.rs b/tests/groups.rs
index 9f3a6eda..4b10678a 100644
--- a/tests/groups.rs
+++ b/tests/groups.rs
@@ -45,13 +45,8 @@ fn required_group_missing_arg() {
     assert_eq!(err.kind, ErrorKind::MissingRequiredArgument);
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
-// #[cfg(debug_assertions)]
 #[test]
-// This used to provide a nice, programmer-friendly error.
-// Now the error directs the programmer to file a bug report with clap.
-// #[should_panic(expected = "The group 'req' contains the arg 'flg' that doesn't actually exist.")]
-#[should_panic(expected = "internal error")]
+#[should_panic]
 fn non_existing_arg() {
     let _ = App::new("group")
         .arg("-f, --flag 'some flag'")
diff --git a/tests/help.rs b/tests/help.rs
index 13f2b787..6a1c6506 100644
--- a/tests/help.rs
+++ b/tests/help.rs
@@ -4,7 +4,7 @@ extern crate regex;
 
 include!("../clap-test.rs");
 
-use clap::{App, AppSettings, Arg, ArgGroup, ArgSettings, ErrorKind};
+use clap::{App, AppSettings, Arg, ArgSettings, ErrorKind};
 
 static REQUIRE_DELIM_HELP: &'static str = "test 1.3
 Kevin K.
@@ -532,21 +532,6 @@ OPTIONS:
 NETWORKING:
     -n, --no-proxy    Do not use system proxy settings";
 
-static ISSUE_1487: &'static str = "test 
-
-USAGE:
-    ctest <arg1|arg2>
-
-ARGS:
-    <arg1>    
-    <arg2>    
-
-FLAGS:
-    -h, --help       Prints help information
-    -V, --version    Prints version information";
-
-
-
 fn setup() -> App<'static> {
     App::new("test")
         .author("Kevin K.")
@@ -1520,16 +1505,3 @@ fn show_short_about_issue_897() {
         false
     ));
 }
-
-#[test]
-fn issue_1487() {
-    let app = App::new("test")
-        .arg(Arg::with_name("arg1")
-            .group("group1"))
-        .arg(Arg::with_name("arg2")
-            .group("group1"))
-        .group(ArgGroup::with_name("group1")
-            .args(&["arg1", "arg2"])
-            .required(true));
-    assert!(test::compare_output(app, "ctest -h", ISSUE_1487, false));
-} 
diff --git a/tests/multiple_values.rs b/tests/multiple_values.rs
index 001adafa..aad11565 100644
--- a/tests/multiple_values.rs
+++ b/tests/multiple_values.rs
@@ -963,13 +963,8 @@ fn req_delimiter_complex() {
     );
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
-#[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "When using a positional argument with \
-.multiple(true) that is *not the last* positional argument, the last \
-positional argument (i.e the one with the highest index) *must* have \
-.required(true) or .last(true) set.")]
+#[should_panic]
 fn low_index_positional_not_required() {
     let _ = App::new("lip")
         .arg(
@@ -979,14 +974,11 @@ fn low_index_positional_not_required() {
                 .multiple(true),
         )
         .arg(Arg::with_name("target").index(2))
-        .try_get_matches_from(vec![""]);
+        .try_get_matches_from(vec!["lip", "file1", "file2", "file3", "target"]);
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
-#[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "Only one positional argument with .multiple(true) \
-set is allowed per command, unless the second one also has .last(true) set")]
+#[should_panic]
 fn low_index_positional_last_multiple_too() {
     let _ = App::new("lip")
         .arg(
@@ -1001,14 +993,11 @@ fn low_index_positional_last_multiple_too() {
                 .required(true)
                 .multiple(true),
         )
-        .try_get_matches_from(vec![""]);
+        .try_get_matches_from(vec!["lip", "file1", "file2", "file3", "target"]);
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
-#[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "Only the last positional argument, or second to \
-last positional argument may be set to .multiple(true)")]
+#[should_panic]
 fn low_index_positional_too_far_back() {
     let _ = App::new("lip")
         .arg(
@@ -1019,7 +1008,7 @@ fn low_index_positional_too_far_back() {
         )
         .arg(Arg::with_name("target").required(true).index(2))
         .arg(Arg::with_name("target2").required(true).index(3))
-        .try_get_matches_from(vec![""]);
+        .try_get_matches_from(vec!["lip", "file1", "file2", "file3", "target"]);
 }
 
 #[test]
diff --git a/tests/opts.rs b/tests/opts.rs
index 6c38d336..6016f9f8 100644
--- a/tests/opts.rs
+++ b/tests/opts.rs
@@ -9,7 +9,6 @@ use clap::{App, Arg, ArgMatches, ArgSettings, ErrorKind};
 static DYM: &'static str =
     "error: Found argument '--optio' which wasn't expected, or isn't valid in this context
 \tDid you mean --option?
-If you tried to supply `--optio` as a PATTERN use `-- --optio`
 
 USAGE:
     clap-test --option <opt>...
diff --git a/tests/positionals.rs b/tests/positionals.rs
index 65e95b47..d3b0f2a8 100644
--- a/tests/positionals.rs
+++ b/tests/positionals.rs
@@ -218,16 +218,15 @@ fn single_positional_required_usage_string() {
     assert_eq!(app.generate_usage(), "USAGE:\n    test <FILE>");
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
-#[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "Found positional argument which is not required \
-with a lower index than a required positional argument")]
+#[should_panic]
 fn missing_required() {
-    let _ = App::new("test")
+    let r = App::new("test")
         .arg("[FILE1] 'some file'")
         .arg("<FILE2> 'some file'")
-        .try_get_matches_from(vec![""]);
+        .try_get_matches_from(vec!["test", "file"]);
+    assert!(r.is_err());
+    assert_eq!(r.unwrap_err().kind, ErrorKind::MissingRequiredArgument);
 }
 
 #[test]
diff --git a/tests/propagate_globals.rs b/tests/propagate_globals.rs
index cdd23241..961f1f5d 100644
--- a/tests/propagate_globals.rs
+++ b/tests/propagate_globals.rs
@@ -30,12 +30,12 @@ mod tests {
         app.get_matches_from(argv.split(" ").collect::<Vec<_>>())
     }
 
-    fn get_outer_matches(m: &ArgMatches) -> &ArgMatches {
+    fn get_outer_matches<'a>(m: &'a ArgMatches) -> &'a ArgMatches {
         m.subcommand_matches("outer")
             .expect("could not access outer subcommand")
     }
 
-    fn get_inner_matches(m: &ArgMatches) -> &ArgMatches {
+    fn get_inner_matches<'a>(m: &'a ArgMatches) -> &'a ArgMatches {
         get_outer_matches(m)
             .subcommand_matches("inner")
             .expect("could not access inner subcommand")
diff --git a/tests/subcommands.rs b/tests/subcommands.rs
index 5959f282..c2036a01 100644
--- a/tests/subcommands.rs
+++ b/tests/subcommands.rs
@@ -46,7 +46,6 @@ For more information try --help";
 static DYM_ARG: &'static str =
     "error: Found argument '--subcm' which wasn't expected, or isn't valid in this context
 \tDid you mean to put '--subcmdarg' after the subcommand 'subcmd'?
-If you tried to supply `--subcm` as a PATTERN use `-- --subcm`
 
 USAGE:
     dym [SUBCOMMAND]
diff --git a/tests/unique_args.rs b/tests/unique_args.rs
index 6f3d41e8..b24f85fd 100644
--- a/tests/unique_args.rs
+++ b/tests/unique_args.rs
@@ -2,23 +2,19 @@ extern crate clap;
 
 use clap::{App, Arg};
 
-// This tests a programmer error and will only succeed with debug_assertions
-#[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "Arg names must be unique")]
+#[should_panic]
 fn unique_arg_names() {
     let _ = App::new("some")
         .args(&[
-            Arg::with_name("arg1").short('a'),
-            Arg::with_name("arg1").short('b'),
+            Arg::with_name("arg").short('a'),
+            Arg::with_name("arg").short('b'),
         ])
         .try_get_matches();
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
-#[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "Argument short must be unique")]
+#[should_panic]
 fn unique_arg_shorts() {
     let _ = App::new("some")
         .args(&[
@@ -28,10 +24,8 @@ fn unique_arg_shorts() {
         .try_get_matches();
 }
 
-// This tests a programmer error and will only succeed with debug_assertions
-#[cfg(debug_assertions)]
 #[test]
-#[should_panic(expected = "Argument long must be unique")]
+#[should_panic]
 fn unique_arg_longs() {
     let _ = App::new("some")
         .args(&[