Repository: clap-rs/clap (rust)

Issue:
`validator_os` takes `&OsStr`, but `validator` takes `String`
If find it somewhat inconsistent that one function borrows the value, while the other takes it by value. It is very unlikely that anyone just forwards the argument passed to the `Err` return value, so there's no benefit of offering users an owned `String`.

Full disclosure: It clashes with a clippy lint, and I don't know how to act on it: https://github.com/rust-lang-nursery/rust-clippy/issues/2434

Pull request:
Make validator take &str instead of String
<!--
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 it's own line.
-->
Closes #1165 

Patch:
diff --git a/benches/05_ripgrep.rs b/benches/05_ripgrep.rs
index 6fa12e77..7a5f2858 100644
--- a/benches/05_ripgrep.rs
+++ b/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())
diff --git a/clap_derive/src/derives/into_app.rs b/clap_derive/src/derives/into_app.rs
index b42d55b3..38a64204 100644
--- a/clap_derive/src/derives/into_app.rs
+++ b/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!(),
                 };
diff --git a/examples/15_custom_validator.rs b/examples/15_custom_validator.rs
index 976816eb..e8255950 100644
--- a/examples/15_custom_validator.rs
+++ b/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") {
diff --git a/examples/18_builder_macro.rs b/examples/18_builder_macro.rs
index ddc355cc..38bc2973 100644
--- a/examples/18_builder_macro.rs
+++ b/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 {
diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs
index 2cebbd4c..3f4136ed 100644
--- a/src/build/app/mod.rs
+++ b/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 {
diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs
index 8024595f..7a92a037 100644
--- a/src/build/arg/mod.rs
+++ b/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| {
diff --git a/src/parse/errors.rs b/src/parse/errors.rs
index 2fde154f..c5215d19 100644
--- a/src/parse/errors.rs
+++ b/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!")),
diff --git a/src/parse/validator.rs b/src/parse/validator.rs
index c03ccad8..84571287 100644
--- a/src/parse/validator.rs
+++ b/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 {


Test patch:
diff --git a/tests/validators.rs b/tests/validators.rs
new file mode 100644
index 00000000..5abedf6c
--- /dev/null
+++ b/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"]);
+}