refactor value parsing

derive/src/argument.rs

@@ -358,19 +358,19 @@ fn default_value_expression(ident: &Ident, default_expr: &TokenStream) -> TokenS
 
 fn optional_value_expression(ident: &Ident, default_expr: &TokenStream) -> TokenStream {
     quote!(match parser.optional_value() {
-        Some(value) => Self::#ident(FromValue::from_value(&option, value)?),
+        Some(value) => Self::#ident(::uutils_args::parse_value_for_option(&option, &value)?),
         None => Self::#ident(#default_expr),
     })
 }
 
 fn required_value_expression(ident: &Ident) -> TokenStream {
-    quote!(Self::#ident(FromValue::from_value(&option, parser.value()?)?))
+    quote!(Self::#ident(::uutils_args::parse_value_for_option(&option, &parser.value()?)?))
 }
 
 fn positional_expression(ident: &Ident) -> TokenStream {
     // TODO: Add option name in this from_value call
     quote!(
-        Self::#ident(FromValue::from_value("", value)?)
+        Self::#ident(::uutils_args::parse_value_for_option("", &value)?)
     )
 }
 
@@ -380,7 +380,7 @@ fn last_positional_expression(ident: &Ident) -> TokenStream {
         let raw_args = parser.raw_args()?;
         let collection = std::iter::once(value)
             .chain(raw_args)
-            .map(|v| FromValue::from_value("", v))
+            .map(|v| ::uutils_args::parse_value_for_option("", &v))
             .collect::<Result<_,_>>()?;
         Self::#ident(collection)
     })

derive/src/field.rs

@@ -22,7 +22,7 @@ pub(crate) fn parse_field(field: &Field) -> FieldData {
     if let Some(env_var) = field_attr.env {
         default_value = quote!(
             ::std::env::var_os(#env_var)
-                .and_then(|v| ::uutils_args::FromValue::from_value("", v).ok())
+                .and_then(|v| ::uutils_args::Value::from_value(&v).ok())
                 .unwrap_or(#default_value)
         )
     }

derive/src/lib.rs

@@ -114,7 +114,7 @@ pub fn arguments(input: TokenStream) -> TokenStream {
             fn next_arg(
                 parser: &mut uutils_args::lexopt::Parser, positional_idx: &mut usize
             ) -> Result<Option<uutils_args::Argument<Self>>, uutils_args::Error> {
-                use uutils_args::{FromValue, lexopt, Error, Argument};
+                use uutils_args::{Value, lexopt, Error, Argument};
 
                 #number_argument
 
@@ -151,8 +151,8 @@ pub fn arguments(input: TokenStream) -> TokenStream {
     TokenStream::from(expanded)
 }
 
-#[proc_macro_derive(FromValue, attributes(value))]
-pub fn from_value(input: TokenStream) -> TokenStream {
+#[proc_macro_derive(Value, attributes(value))]
+pub fn value(input: TokenStream) -> TokenStream {
     let input = parse_macro_input!(input as DeriveInput);
 
     let name = input.ident;
@@ -195,9 +195,9 @@ pub fn from_value(input: TokenStream) -> TokenStream {
     }
 
     let expanded = quote!(
-        impl #impl_generics FromValue for #name #ty_generics #where_clause {
-            fn from_value(option: &str, value: std::ffi::OsString) -> Result<Self, uutils_args::Error> {
-                let value = String::from_value(option, value)?;
+        impl #impl_generics Value for #name #ty_generics #where_clause {
+            fn from_value(value: &::std::ffi::OsStr) -> ::uutils_args::ValueResult<Self> {
+                let value = String::from_value(value)?;
                 let options: &[&[&str]] = &[#(#options),*];
                 let mut candidates: Vec<&str> = Vec::new();
                 let mut exact_match: Option<&str> = None;
@@ -217,16 +217,11 @@ pub fn from_value(input: TokenStream) -> TokenStream {
                 let opt = match (exact_match, &candidates[..]) {
                     (Some(opt), _) => opt,
                     (None, [opt]) => opt,
-                    (None, []) => return Err(uutils_args::Error::ParsingFailed {
-                        option: option.to_string(),
-                        value,
-                        error: "Invalid value".into(),
-                    }),
-                    (None, opts) => return Err(uutils_args::Error::AmbiguousValue {
-                        option: option.to_string(),
+                    (None, []) => return Err("Invalid value".into()),
+                    (None, opts) => return Err(uutils_args::ValueError::AmbiguousValue {
                         value,
                         candidates: candidates.iter().map(|s| s.to_string()).collect(),
-                    })
+                    }.into())
                 };
 
                 Ok(match opt {

src/error.rs

@@ -35,19 +35,8 @@ pub enum Error {
         candidates: Vec<String>,
     },
 
-    /// An abbreviated value was given that could match multiple values.
-    AmbiguousValue {
-        option: String,
-        value: String,
-        candidates: Vec<String>,
-    },
-
     /// The value was required to be valid UTF-8, but it wasn't.
     NonUnicodeValue(OsString),
-
-    /// Some custom error, probably returned by a
-    /// [`FromValue`](crate::FromValue) implementation.
-    Custom(Box<dyn StdError + Send + Sync + 'static>),
 }
 
 impl StdError for Error {}
@@ -91,13 +80,12 @@ impl Display for Error {
                 value,
                 error,
             } => {
+                // TODO: option should not not be Option<String>, because even for positional
+                // arguments we want to specify the name of the value.
                 if option.is_empty() {
-                    write!(f, "Could not parse value '{value}': {error}")
+                    write!(f, "Invalid value '{value}': {error}")
                 } else {
-                    write!(
-                        f,
-                        "Could not parse value '{value}' for option '{option}': {error}"
-                    )
+                    write!(f, "Invalid value '{value}' for '{option}': {error}")
                 }
             }
             Error::AmbiguousOption { option, candidates } => {
@@ -110,24 +98,9 @@ impl Display for Error {
                 }
                 Ok(())
             }
-            Error::AmbiguousValue {
-                option,
-                value,
-                candidates,
-            } => {
-                write!(
-                    f,
-                    "Value '{value}' for option '{option}' is ambiguous. The following candidates match:",
-                )?;
-                for candidate in candidates {
-                    write!(f, "  - {candidate}")?;
-                }
-                Ok(())
-            }
             Error::NonUnicodeValue(x) => {
                 write!(f, "Invalid unicode value found: {}", x.to_string_lossy())
             }
-            Error::Custom(err) => std::fmt::Display::fmt(err, f),
         }
     }
 }
@@ -141,9 +114,10 @@ impl From<lexopt::Error> for Error {
             lexopt::Error::UnexpectedValue { option, value } => {
                 Self::UnexpectedValue { option, value }
             }
-            lexopt::Error::ParsingFailed { .. } => panic!("Conversion not supported"),
             lexopt::Error::NonUnicodeValue(s) => Self::NonUnicodeValue(s),
-            lexopt::Error::Custom(e) => Self::Custom(e),
+            lexopt::Error::ParsingFailed { .. } | lexopt::Error::Custom(_) => {
+                panic!("Should never be constructed.")
+            }
         }
     }
 }

src/lib.rs

@@ -121,8 +121,8 @@
 //!
 //! - [`Initial`] is an alternative to the [`Default`] trait from the standard
 //!   library, with a richer derive macro.
-//! - [`FromValue`] allows for easy parsing from `OsStr` to any type
-//!   implementing [`FromValue`]. This crate also provides a derive macro for
+//! - [`Value`] allows for easy parsing from `OsStr` to any type
+//!   implementing [`Value`]. This crate also provides a derive macro for
 //!   this trait.
 //!
 //! # Examples
@@ -141,15 +141,19 @@
 //! - [mktemp](https://github.com/tertsdiepraam/uutils-args/blob/main/tests/coreutils/mktemp.rs)
 
 mod error;
+mod value;
+
 pub use derive::*;
 pub use lexopt;
 pub use term_md;
 
 pub use error::Error;
-use std::ffi::OsStr;
-use std::num::ParseIntError;
-use std::path::PathBuf;
-use std::{ffi::OsString, marker::PhantomData};
+pub use value::{Value, ValueError, ValueResult};
+
+use std::{
+    ffi::{OsStr, OsString},
+    marker::PhantomData,
+};
 
 /// A wrapper around a type implementing [`Arguments`] that adds `Help`
 /// and `Version` variants.
@@ -352,72 +356,6 @@ pub trait Options: Sized + Initial {
     }
 }
 
-/// Defines how a type should be parsed from an argument.
-pub trait FromValue: Sized {
-    fn from_value(option: &str, value: OsString) -> Result<Self, Error>;
-}
-
-impl FromValue for OsString {
-    fn from_value(_option: &str, value: OsString) -> Result<Self, Error> {
-        Ok(value)
-    }
-}
-
-impl FromValue for PathBuf {
-    fn from_value(_option: &str, value: OsString) -> Result<Self, Error> {
-        Ok(PathBuf::from(value))
-    }
-}
-
-impl FromValue for String {
-    fn from_value(_option: &str, value: OsString) -> Result<Self, Error> {
-        match value.into_string() {
-            Ok(s) => Ok(s),
-            Err(os) => Err(Error::NonUnicodeValue(os)),
-        }
-    }
-}
-
-impl<T> FromValue for Option<T>
-where
-    T: FromValue,
-{
-    fn from_value(option: &str, value: OsString) -> Result<Self, Error> {
-        Ok(Some(T::from_value(option, value)?))
-    }
-}
-
-macro_rules! from_value_int {
-    ($t: ty) => {
-        impl FromValue for $t {
-            fn from_value(option: &str, value: OsString) -> Result<Self, Error> {
-                let value = String::from_value(option, value)?;
-                value
-                    .parse()
-                    .map_err(|e: ParseIntError| Error::ParsingFailed {
-                        value,
-                        option: option.to_string(),
-                        error: e.into(),
-                    })
-            }
-        }
-    };
-}
-
-from_value_int!(u8);
-from_value_int!(u16);
-from_value_int!(u32);
-from_value_int!(u64);
-from_value_int!(u128);
-from_value_int!(usize);
-
-from_value_int!(i8);
-from_value_int!(i16);
-from_value_int!(i32);
-from_value_int!(i64);
-from_value_int!(i128);
-from_value_int!(isize);
-
 /// Parses an echo-style positional argument
 ///
 /// This means that any argument that does not solely consist of a hyphen
@@ -453,13 +391,16 @@ fn is_echo_style_positional(s: &OsStr, short_args: &[char]) -> bool {
     !is_short_args
 }
 
-pub fn parse_prefix<T: FromValue>(parser: &mut lexopt::Parser, prefix: &'static str) -> Option<T> {
+/// Parse an argument defined by a prefix
+#[doc(hidden)]
+pub fn parse_prefix<T: Value>(parser: &mut lexopt::Parser, prefix: &'static str) -> Option<T> {
     let mut raw = parser.try_raw_args()?;
+
+    // TODO: The to_str call is a limitation. Maybe we need to pull in something like bstr
     let arg = raw.peek()?.to_str()?;
     let value_str = arg.strip_prefix(prefix)?;
 
-    // TODO: Give a nice flag name
-    let value = T::from_value("", OsString::from(value_str)).ok()?;
+    let value = T::from_value(OsStr::new(value_str)).ok()?;
 
     // Consume the argument we just parsed
     let _ = raw.next();
@@ -467,6 +408,16 @@ pub fn parse_prefix<T: FromValue>(parser: &mut lexopt::Parser, prefix: &'static
     Some(value)
 }
 
+/// Parse a value and wrap the error into an `Error::ParsingFailed`
+#[doc(hidden)]
+pub fn parse_value_for_option<T: Value>(opt: &str, v: &OsStr) -> Result<T, Error> {
+    T::from_value(v).map_err(|e| Error::ParsingFailed {
+        option: opt.into(),
+        value: v.to_string_lossy().to_string(),
+        error: e,
+    })
+}
+
 #[cfg(test)]
 mod test {
     use std::ffi::OsStr;

src/value.rs

@@ -0,0 +1,106 @@
+use crate::error::Error;
+use std::{
+    ffi::{OsStr, OsString},
+    path::PathBuf,
+};
+
+pub type ValueResult<T> = Result<T, Box<dyn std::error::Error + Send + Sync + 'static>>;
+
+pub enum ValueError {
+    /// An abbreviated value was given that could match multiple values.
+    AmbiguousValue {
+        value: String,
+        candidates: Vec<String>,
+    },
+    InvalidUnicode(OsString),
+}
+
+impl std::error::Error for ValueError {}
+
+impl std::fmt::Debug for ValueError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        std::fmt::Display::fmt(self, f)
+    }
+}
+
+impl std::fmt::Display for ValueError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            ValueError::AmbiguousValue { value, candidates } => {
+                write!(
+                    f,
+                    "Value '{value}' is ambiguous. The following candidates match:"
+                )?;
+                for candidate in candidates {
+                    write!(f, "  - {candidate}")?;
+                }
+                Ok(())
+            }
+            ValueError::InvalidUnicode(x) => {
+                write!(f, "'{}' is invalid unicode.", x.to_string_lossy())
+            }
+        }
+    }
+}
+
+/// Defines how a type should be parsed from an argument.
+///
+/// If an error is returned, it will be wrapped in [`Error::ParsingFailed`]
+pub trait Value: Sized {
+    fn from_value(value: &OsStr) -> ValueResult<Self>;
+}
+
+impl Value for OsString {
+    fn from_value(value: &OsStr) -> ValueResult<Self> {
+        Ok(value.into())
+    }
+}
+
+impl Value for PathBuf {
+    fn from_value(value: &OsStr) -> ValueResult<Self> {
+        Ok(PathBuf::from(value))
+    }
+}
+
+impl Value for String {
+    fn from_value(value: &OsStr) -> ValueResult<Self> {
+        match value.to_str() {
+            Some(s) => Ok(s.into()),
+            None => Err(Error::NonUnicodeValue(value.into()).into()),
+        }
+    }
+}
+
+impl<T> Value for Option<T>
+where
+    T: Value,
+{
+    fn from_value(value: &OsStr) -> ValueResult<Self> {
+        Ok(Some(T::from_value(value)?))
+    }
+}
+
+macro_rules! value_int {
+    ($t: ty) => {
+        impl Value for $t {
+            fn from_value(value: &OsStr) -> ValueResult<Self> {
+                let string = String::from_value(value)?;
+                Ok(string.parse()?)
+            }
+        }
+    };
+}
+
+value_int!(u8);
+value_int!(u16);
+value_int!(u32);
+value_int!(u64);
+value_int!(u128);
+value_int!(usize);
+
+value_int!(i8);
+value_int!(i16);
+value_int!(i32);
+value_int!(i64);
+value_int!(i128);
+value_int!(isize);

tests/coreutils/ls.rs

@@ -1,7 +1,7 @@
 use std::path::PathBuf;
-use uutils_args::{Arguments, FromValue, Initial, Options};
+use uutils_args::{Arguments, Initial, Options, Value};
 
-#[derive(Default, Debug, PartialEq, Eq, FromValue)]
+#[derive(Default, Debug, PartialEq, Eq, Value)]
 enum Format {
     #[value("long")]
     Long,
@@ -20,7 +20,7 @@ enum Format {
     Commas,
 }
 
-#[derive(Default, Debug, PartialEq, Eq, FromValue)]
+#[derive(Default, Debug, PartialEq, Eq, Value)]
 enum When {
     #[value("yes", "always", "force")]
     Always,
@@ -60,7 +60,7 @@ enum Dereference {
     All,
 }
 
-#[derive(Default, Debug, PartialEq, Eq, FromValue)]
+#[derive(Default, Debug, PartialEq, Eq, Value)]
 enum QuotingStyle {
     #[value("literal")]
     Literal,
@@ -85,7 +85,7 @@ enum QuotingStyle {
     Escape,
 }
 
-#[derive(Default, Debug, PartialEq, Eq, FromValue)]
+#[derive(Default, Debug, PartialEq, Eq, Value)]
 enum Sort {
     #[default]
     Name,
@@ -103,7 +103,7 @@ enum Sort {
     Width,
 }
 
-#[derive(Default, Debug, PartialEq, Eq, FromValue)]
+#[derive(Default, Debug, PartialEq, Eq, Value)]
 enum Time {
     #[default]
     Modification,
@@ -115,7 +115,7 @@ enum Time {
     Birth,
 }
 
-#[derive(Default, Debug, FromValue, PartialEq, Eq)]
+#[derive(Default, Debug, Value, PartialEq, Eq)]
 enum IndicatorStyle {
     #[default]
     #[value("none")]

tests/options.rs

@@ -1,6 +1,6 @@
-use std::ffi::OsString;
+use std::ffi::OsStr;
 
-use uutils_args::{Arguments, FromValue, Initial, Options};
+use uutils_args::{Arguments, Initial, Options, Value, ValueResult};
 
 #[test]
 fn string_option() {
@@ -30,7 +30,7 @@ fn string_option() {
 
 #[test]
 fn enum_option() {
-    #[derive(FromValue, Default, Debug, PartialEq, Eq, Clone)]
+    #[derive(Value, Default, Debug, PartialEq, Eq, Clone)]
     enum Format {
         #[default]
         #[value]
@@ -72,7 +72,7 @@ fn enum_option() {
 
 #[test]
 fn enum_option_with_fields() {
-    #[derive(FromValue, Default, Debug, PartialEq, Eq)]
+    #[derive(Value, Default, Debug, PartialEq, Eq)]
     enum Indent {
         #[default]
         Tabs,
@@ -118,19 +118,15 @@ fn enum_with_complex_from_value() {
         Spaces(u8),
     }
 
-    impl FromValue for Indent {
-        fn from_value(option: &str, value: std::ffi::OsString) -> Result<Self, uutils_args::Error> {
-            let value = String::from_value(option, value)?;
+    impl Value for Indent {
+        fn from_value(value: &std::ffi::OsStr) -> ValueResult<Self> {
+            let value = String::from_value(value)?;
             if value == "tabs" {
                 Ok(Self::Tabs)
             } else if let Ok(n) = value.parse() {
                 Ok(Self::Spaces(n))
             } else {
-                Err(uutils_args::Error::ParsingFailed {
-                    option: option.to_string(),
-                    value,
-                    error: "Failure!".into(),
-                })
+                Err("Failure!".into())
             }
         }
     }
@@ -159,7 +155,7 @@ fn enum_with_complex_from_value() {
 
 #[test]
 fn color() {
-    #[derive(Default, FromValue, Debug, PartialEq, Eq)]
+    #[derive(Default, Value, Debug, PartialEq, Eq)]
     enum Color {
         #[value("yes", "always")]
         Always,
@@ -336,7 +332,7 @@ fn integers() {
 
 #[test]
 fn ls_classify() {
-    #[derive(FromValue, Default, PartialEq, Eq, Debug)]
+    #[derive(Value, Default, PartialEq, Eq, Debug)]
     enum When {
         #[value]
         Never,
@@ -418,7 +414,7 @@ fn mktemp_tmpdir() {
 
 #[test]
 fn infer_value() {
-    #[derive(FromValue, PartialEq, Eq, Debug)]
+    #[derive(Value, PartialEq, Eq, Debug)]
     enum Foo {
         #[value("long")]
         Long,
@@ -430,17 +426,11 @@ fn infer_value() {
         Desk,
     }
 
-    assert_eq!(
-        Foo::from_value("--foo", OsString::from("lo")).unwrap(),
-        Foo::Long
-    );
-    assert_eq!(
-        Foo::from_value("--foo", OsString::from("dec")).unwrap(),
-        Foo::Deck
-    );
+    assert_eq!(Foo::from_value(OsStr::new("lo")).unwrap(), Foo::Long);
+    assert_eq!(Foo::from_value(OsStr::new("dec")).unwrap(), Foo::Deck);
 
-    Foo::from_value("--foo", OsString::from("l")).unwrap_err();
-    Foo::from_value("--foo", OsString::from("de")).unwrap_err();
+    Foo::from_value(OsStr::new("l")).unwrap_err();
+    Foo::from_value(OsStr::new("de")).unwrap_err();
 }
 
 #[test]