Merge #1884

1884: Make validator take &str instead of String r=pksunkara a=CreepySkeleton



Co-authored-by: CreepySkeleton <creepy-skeleton@yandex.ru>

benches/05_ripgrep.rs

@@ -934,7 +934,7 @@ lazy_static! {
     };
 }
 
-fn validate_number(s: String) -> Result<(), String> {
+fn validate_number(s: &str) -> Result<(), String> {
     s.parse::<usize>()
         .map(|_| ())
         .map_err(|err| err.to_string())

clap_derive/src/derives/into_app.rs

@@ -205,13 +205,13 @@ pub fn gen_app_augmentation(
                     _ if attrs.is_enum() => quote!(),
                     ParserKind::TryFromStr => quote_spanned! { func.span()=>
                         .validator(|s| {
-                            #func(s.as_str())
+                            #func(s)
                             .map(|_: #convert_type| ())
                             .map_err(|e| e.to_string())
                         })
                     },
                     ParserKind::TryFromOsStr => quote_spanned! { func.span()=>
-                        .validator_os(|s| #func(&s).map(|_: #convert_type| ()))
+                        .validator_os(|s| #func(s).map(|_: #convert_type| ()))
                     },
                     _ => quote!(),
                 };

examples/15_custom_validator.rs

@@ -21,7 +21,7 @@ fn main() {
     println!("The .PNG file is: {}", matches.value_of("input").unwrap());
 }
 
-fn is_png(val: String) -> Result<(), String> {
+fn is_png(val: &str) -> Result<(), String> {
     // val is the argument value passed in by the user
     // val has type of String.
     if val.ends_with(".png") {

examples/18_builder_macro.rs

@@ -5,7 +5,7 @@ use clap::clap_app;
 
 fn main() {
     // Validation example testing that a file exists
-    let file_exists = |path| {
+    let file_exists = |path: &str| {
         if std::fs::metadata(path).is_ok() {
             Ok(())
         } else {

src/build/app/mod.rs

@@ -1711,6 +1711,13 @@ impl<'b> App<'b> {
                 "Global arguments cannot be required.\n\n\t'{}' is marked as both global and required",
                 arg.name
             );
+
+            // validators
+            assert!(
+                arg.validator.is_none() || arg.validator_os.is_none(),
+                "Argument '{}' has both `validator` and `validator_os` set which is not allowed",
+                arg.name
+            );
         }
 
         for group in &self.groups {

src/build/arg/mod.rs

@@ -25,7 +25,7 @@ use crate::{
     INTERNAL_ERROR_MSG,
 };
 
-type Validator = Rc<dyn Fn(String) -> Result<(), String>>;
+type Validator = Rc<dyn Fn(&str) -> Result<(), String>>;
 type ValidatorOs = Rc<dyn Fn(&OsStr) -> Result<(), String>>;
 
 /// The abstract representation of a command line argument. Used to set all the options and
@@ -1927,7 +1927,7 @@ impl<'help> Arg<'help> {
     ///
     /// ```rust
     /// # use clap::{App, Arg};
-    /// fn has_at(v: String) -> Result<(), String> {
+    /// fn has_at(v: &str) -> Result<(), String> {
     ///     if v.contains("@") { return Ok(()); }
     ///     Err(String::from("The value did not contain the required @ sigil"))
     /// }
@@ -1947,7 +1947,7 @@ impl<'help> Arg<'help> {
     /// [`Rc`]: https://doc.rust-lang.org/std/rc/struct.Rc.html
     pub fn validator<F, O, E>(mut self, f: F) -> Self
     where
-        F: Fn(String) -> Result<O, E> + 'static,
+        F: Fn(&str) -> Result<O, E> + 'static,
         E: ToString,
     {
         self.validator = Some(Rc::new(move |s| {

src/parse/errors.rs

@@ -130,7 +130,7 @@ pub enum ErrorKind {
     ///
     /// ```rust
     /// # use clap::{App, Arg, ErrorKind};
-    /// fn is_numeric(val: String) -> Result<(), String> {
+    /// fn is_numeric(val: &str) -> Result<(), String> {
     ///     match val.parse::<i64>() {
     ///         Ok(..) => Ok(()),
     ///         Err(..) => Err(String::from("Value wasn't a number!")),

src/parse/validator.rs

@@ -137,7 +137,7 @@ impl<'b, 'c, 'z> Validator<'b, 'c, 'z> {
             }
             if let Some(ref vtor) = arg.validator {
                 debug!("Validator::validate_arg_values: checking validator...");
-                if let Err(e) = vtor(val.to_string_lossy().into_owned()) {
+                if let Err(e) = vtor(&*val.to_string_lossy()) {
                     debug!("error");
                     return Err(Error::value_validation(Some(arg), &e, self.p.app.color())?);
                 } else {

tests/validators.rs

@@ -0,0 +1,19 @@
+use clap::{App, Arg};
+
+#[cfg(debug_assertions)]
+#[test]
+#[should_panic = "Argument 'test' has both `validator` and `validator_os` set which is not allowed"]
+fn both_validator_and_validator_os() {
+    let _ = App::new("test")
+        .arg(
+            Arg::with_name("test")
+                .validator(|val| val.parse::<u32>().map_err(|e| e.to_string()))
+                .validator_os(|val| {
+                    val.to_str()
+                        .unwrap()
+                        .parse::<u32>()
+                        .map_err(|e| e.to_string())
+                }),
+        )
+        .try_get_matches_from(&["app", "1"]);
+}