refactor(flags): refactor parsing arguments

- Make code more idiomatic and readable and have the same pattern
- Use `unreachable` to clearly show internal logic error
- Rename function `from_str` to `from_arg_str` to make it more specific

src/flags/blocks.rs

@@ -71,24 +71,24 @@ impl Blocks {
     /// This errors if any of the parameter arguments causes [Block]'s implementation of
     /// [TryFrom::try_from] to return an [Err].
     fn from_arg_matches(matches: &ArgMatches) -> Option<Result<Self, Error>> {
-        if matches.occurrences_of("blocks") > 0 {
-            if let Some(values) = matches.values_of("blocks") {
-                let mut blocks: Vec<Block> = vec![];
-                for value in values {
-                    match Block::try_from(value) {
-                        Ok(block) => blocks.push(block),
-                        Err(message) => {
-                            return Some(Err(Error::with_description(
-                                &message,
-                                ErrorKind::ValueValidation,
-                            )))
-                        }
+        if matches.occurrences_of("blocks") == 0 {
+            return None;
+        }
+
+        if let Some(values) = matches.values_of("blocks") {
+            let mut blocks: Vec<Block> = Vec::with_capacity(values.len());
+            for value in values {
+                match Block::try_from(value) {
+                    Ok(block) => blocks.push(block),
+                    Err(message) => {
+                        return Some(Err(Error::with_description(
+                            &message,
+                            ErrorKind::ValueValidation,
+                        )))
                     }
                 }
-                Some(Ok(Self(blocks)))
-            } else {
-                None
             }
+            Some(Ok(Self(blocks)))
         } else {
             None
         }

src/flags/color.rs

@@ -4,7 +4,6 @@
 use super::Configurable;
 
 use crate::config_file::Config;
-use crate::print_error;
 
 use clap::ArgMatches;
 use serde::de::{self, Deserializer, Visitor};
@@ -45,18 +44,15 @@ pub enum ThemeOption {
 
 impl ThemeOption {
     fn from_config(config: &Config) -> ThemeOption {
-        if let Some(classic) = config.classic {
-            if classic {
-                return ThemeOption::NoColor;
-            }
+        if config.classic == Some(true) {
+            ThemeOption::NoColor
+        } else {
+            config
+                .color
+                .as_ref()
+                .and_then(|c| c.theme.clone())
+                .unwrap_or_default()
         }
-        if let Some(c) = &config.color {
-            if let Some(t) = &c.theme {
-                return t.clone();
-            }
-        }
-
-        ThemeOption::default()
     }
 }
 
@@ -105,18 +101,13 @@ pub enum ColorOption {
 }
 
 impl ColorOption {
-    /// Get a Color value from a [String].
-    fn from_str(value: &str) -> Option<Self> {
+    fn from_arg_str(value: &str) -> Self {
         match value {
-            "always" => Some(Self::Always),
-            "auto" => Some(Self::Auto),
-            "never" => Some(Self::Never),
+            "always" => Self::Always,
+            "auto" => Self::Auto,
+            "never" => Self::Never,
             _ => {
-                print_error!(
-                    "Config color.when could only be one of auto, always and never, got {}.",
-                    &value
-                );
-                None
+                unreachable!("Invalid value should be handled by `clap`");
             }
         }
     }
@@ -132,11 +123,7 @@ impl Configurable<Self> for ColorOption {
         if matches.is_present("classic") {
             Some(Self::Never)
         } else if matches.occurrences_of("color") > 0 {
-            if let Some(color) = matches.values_of("color")?.last() {
-                Self::from_str(color)
-            } else {
-                panic!("Bad color args. This should not be reachable!");
-            }
+            matches.values_of("color")?.last().map(Self::from_arg_str)
         } else {
             None
         }
@@ -148,14 +135,10 @@ impl Configurable<Self> for ColorOption {
     /// Otherwise if the `Config::color::when` has value and is one of "always", "auto" or "never"
     /// this returns its corresponding variant in a [Some]. Otherwise this returns [None].
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(true) = config.classic {
-            return Some(Self::Never);
-        }
-
-        if let Some(c) = &config.color {
-            c.when
+        if config.classic == Some(true) {
+            Some(Self::Never)
         } else {
-            None
+            config.color.as_ref().and_then(|c| c.when)
         }
     }
 

src/flags/date.rs

@@ -30,7 +30,8 @@ impl DateFlag {
     }
 
     /// Get a value from a str.
-    fn from_str(value: &str) -> Option<Self> {
+    fn from_str<S: AsRef<str>>(value: S) -> Option<Self> {
+        let value = value.as_ref();
         match value {
             "date" => Some(Self::Date),
             "relative" => Some(Self::Relative),
@@ -53,14 +54,7 @@ impl Configurable<Self> for DateFlag {
         if matches.is_present("classic") {
             Some(Self::Date)
         } else if matches.occurrences_of("date") > 0 {
-            match matches.values_of("date")?.last() {
-                Some("date") => Some(Self::Date),
-                Some("relative") => Some(Self::Relative),
-                Some(format) if format.starts_with('+') => {
-                    Some(Self::Formatted(format[1..].to_owned()))
-                }
-                _ => panic!("This should not be reachable!"),
-            }
+            matches.values_of("date")?.last().and_then(Self::from_str)
         } else {
             None
         }
@@ -73,14 +67,10 @@ impl Configurable<Self> for DateFlag {
     /// this returns its corresponding variant in a [Some].
     /// Otherwise this returns [None].
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(true) = &config.classic {
-            return Some(Self::Date);
-        }
-
-        if let Some(date) = &config.date {
-            Self::from_str(date)
+        if config.classic == Some(true) {
+            Some(Self::Date)
         } else {
-            None
+            config.date.as_ref().and_then(Self::from_str)
         }
     }
 

src/flags/dereference.rs

@@ -29,7 +29,7 @@ impl Configurable<Self> for Dereference {
     /// If the `Config::dereference` has value, this returns its value
     /// as the value of the `Dereference`, in a [Some], Otherwise this returns [None].
     fn from_config(config: &Config) -> Option<Self> {
-        config.dereference.as_ref().map(|deref| Self(*deref))
+        config.dereference.map(Self)
     }
 }
 

src/flags/hyperlink.rs

@@ -17,6 +17,17 @@ pub enum HyperlinkOption {
     Never,
 }
 
+impl HyperlinkOption {
+    fn from_arg_str(value: &str) -> Self {
+        match value {
+            "always" => Self::Always,
+            "auto" => Self::Auto,
+            "never" => Self::Never,
+            _ => unreachable!("Invalid value should be handled by `clap`"),
+        }
+    }
+}
+
 impl Configurable<Self> for HyperlinkOption {
     /// Get a potential `HyperlinkOption` variant from [ArgMatches].
     ///
@@ -27,12 +38,10 @@ impl Configurable<Self> for HyperlinkOption {
         if matches.is_present("classic") {
             Some(Self::Never)
         } else if matches.occurrences_of("hyperlink") > 0 {
-            match matches.values_of("hyperlink")?.last() {
-                Some("always") => Some(Self::Always),
-                Some("auto") => Some(Self::Auto),
-                Some("never") => Some(Self::Never),
-                _ => panic!("This should not be reachable!"),
-            }
+            matches
+                .values_of("hyperlink")?
+                .last()
+                .map(Self::from_arg_str)
         } else {
             None
         }
@@ -45,11 +54,11 @@ impl Configurable<Self> for HyperlinkOption {
     /// this returns its corresponding variant in a [Some].
     /// Otherwise this returns [None].
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(true) = &config.classic {
-            return Some(Self::Never);
+        if config.classic == Some(true) {
+            Some(Self::Never)
+        } else {
+            config.hyperlink
         }
-
-        config.hyperlink
     }
 }
 

src/flags/icons.rs

@@ -45,6 +45,19 @@ pub enum IconOption {
     Never,
 }
 
+impl IconOption {
+    fn from_arg_str(value: &str) -> Self {
+        match value {
+            "always" => Self::Always,
+            "auto" => Self::Auto,
+            "never" => Self::Never,
+            _ => {
+                unreachable!("Invalid value should be handled by `clap`");
+            }
+        }
+    }
+}
+
 impl Configurable<Self> for IconOption {
     /// Get a potential `IconOption` variant from [ArgMatches].
     ///
@@ -55,12 +68,7 @@ impl Configurable<Self> for IconOption {
         if matches.is_present("classic") {
             Some(Self::Never)
         } else if matches.occurrences_of("icon") > 0 {
-            match matches.values_of("icon")?.last() {
-                Some("always") => Some(Self::Always),
-                Some("auto") => Some(Self::Auto),
-                Some("never") => Some(Self::Never),
-                _ => panic!("This should not be reachable!"),
-            }
+            matches.values_of("icon")?.last().map(Self::from_arg_str)
         } else {
             None
         }
@@ -73,14 +81,10 @@ impl Configurable<Self> for IconOption {
     /// this returns its corresponding variant in a [Some].
     /// Otherwise this returns [None].
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(true) = &config.classic {
-            return Some(Self::Never);
-        }
-
-        if let Some(icon) = &config.icons {
-            icon.when
+        if config.classic == Some(true) {
+            Some(Self::Never)
         } else {
-            None
+            config.icons.as_ref().and_then(|icon| icon.when)
         }
     }
 }
@@ -100,6 +104,18 @@ pub enum IconTheme {
     Fancy,
 }
 
+impl IconTheme {
+    fn from_arg_str(value: &str) -> Self {
+        match value {
+            "fancy" => Self::Fancy,
+            "unicode" => Self::Unicode,
+            _ => {
+                unreachable!("Invalid value should be handled by `clap`");
+            }
+        }
+    }
+}
+
 impl Configurable<Self> for IconTheme {
     /// Get a potential `IconTheme` variant from [ArgMatches].
     ///
@@ -107,11 +123,10 @@ impl Configurable<Self> for IconTheme {
     /// [Some]. Otherwise this returns [None].
     fn from_arg_matches(matches: &ArgMatches) -> Option<Self> {
         if matches.occurrences_of("icon-theme") > 0 {
-            match matches.values_of("icon-theme")?.last() {
-                Some("fancy") => Some(Self::Fancy),
-                Some("unicode") => Some(Self::Unicode),
-                _ => panic!("This should not be reachable!"),
-            }
+            matches
+                .values_of("icon-theme")?
+                .last()
+                .map(Self::from_arg_str)
         } else {
             None
         }
@@ -123,12 +138,7 @@ impl Configurable<Self> for IconTheme {
     /// this returns its corresponding variant in a [Some].
     /// Otherwise this returns [None].
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(icon) = &config.icons {
-            if let Some(theme) = icon.theme {
-                return Some(theme);
-            }
-        }
-        None
+        config.icons.as_ref().and_then(|icon| icon.theme)
     }
 }
 

src/flags/permission.rs

@@ -19,15 +19,12 @@ pub enum PermissionFlag {
 }
 
 impl PermissionFlag {
-    fn from_str(value: &str) -> Option<Self> {
+    fn from_arg_str(value: &str) -> Self {
         match value {
-            "rwx" => Some(Self::Rwx),
-            "octal" => Some(Self::Octal),
+            "rwx" => Self::Rwx,
+            "octal" => Self::Octal,
             _ => {
-                panic!(
-                    "Permissions can only be one of rwx or octal, but got {}.",
-                    value
-                );
+                unreachable!("Invalid value should be handled by `serde` or `clap`");
             }
         }
     }
@@ -42,13 +39,15 @@ impl Configurable<Self> for PermissionFlag {
     /// Sets permissions to rwx if classic flag is enabled.
     fn from_arg_matches(matches: &ArgMatches) -> Option<Self> {
         if matches.is_present("classic") {
-            return Some(Self::Rwx);
+            Some(Self::Rwx)
         } else if matches.occurrences_of("permission") > 0 {
-            if let Some(permissions) = matches.values_of("permission")?.last() {
-                return Self::from_str(permissions);
-            }
+            matches
+                .values_of("permission")?
+                .last()
+                .map(Self::from_arg_str)
+        } else {
+            None
         }
-        None
     }
 
     /// Get a potential `PermissionFlag` variant from a [Config].
@@ -58,7 +57,7 @@ impl Configurable<Self> for PermissionFlag {
     /// Otherwise this returns [None].
     /// Sets permissions to rwx if classic flag is enabled.
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(true) = config.classic {
+        if config.classic == Some(true) {
             Some(Self::Rwx)
         } else {
             config.permission

src/flags/size.rs

@@ -21,16 +21,13 @@ pub enum SizeFlag {
 }
 
 impl SizeFlag {
-    fn from_str(value: &str) -> Option<Self> {
+    fn from_arg_str(value: &str) -> Self {
         match value {
-            "default" => Some(Self::Default),
-            "short" => Some(Self::Short),
-            "bytes" => Some(Self::Bytes),
+            "default" => Self::Default,
+            "short" => Self::Short,
+            "bytes" => Self::Bytes,
             _ => {
-                panic!(
-                    "Size can only be one of default, short or bytes, but got {}.",
-                    value
-                );
+                unreachable!("Invalid value should be handled by `clap`");
             }
         }
     }
@@ -44,13 +41,12 @@ impl Configurable<Self> for SizeFlag {
     /// [None].
     fn from_arg_matches(matches: &ArgMatches) -> Option<Self> {
         if matches.is_present("classic") {
-            return Some(Self::Bytes);
+            Some(Self::Bytes)
         } else if matches.occurrences_of("size") > 0 {
-            if let Some(size) = matches.values_of("size")?.last() {
-                return Self::from_str(size);
-            }
+            matches.values_of("size")?.last().map(Self::from_arg_str)
+        } else {
+            None
         }
-        None
     }
 
     /// Get a potential `SizeFlag` variant from a [Config].
@@ -59,7 +55,7 @@ impl Configurable<Self> for SizeFlag {
     /// this returns the corresponding `SizeFlag` variant in a [Some].
     /// Otherwise this returns [None].
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(true) = config.classic {
+        if config.classic == Some(true) {
             Some(Self::Bytes)
         } else {
             config.size

src/flags/sorting.rs

@@ -51,10 +51,8 @@ impl Configurable<Self> for SortColumn {
     /// If either the "timesort" or "sizesort" arguments are passed, this returns the corresponding
     /// `SortColumn` variant in a [Some]. Otherwise this returns [None].
     fn from_arg_matches(matches: &ArgMatches) -> Option<Self> {
-        let sort = match matches.values_of("sort") {
-            Some(s) => s.last(),
-            None => None,
-        };
+        let sort = matches.values_of("sort").and_then(|s| s.last());
+
         if matches.is_present("timesort") || sort == Some("time") {
             Some(Self::Time)
         } else if matches.is_present("sizesort") || sort == Some("size") {
@@ -76,11 +74,7 @@ impl Configurable<Self> for SortColumn {
     /// this returns the corresponding variant in a [Some].
     /// Otherwise this returns [None].
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(sort) = &config.sorting {
-            sort.column
-        } else {
-            None
-        }
+        config.sorting.as_ref().and_then(|s| s.column)
     }
 }
 
@@ -118,19 +112,11 @@ impl Configurable<Self> for SortOrder {
     /// Otherwise [None] is returned.
     /// A `true` maps to [SortOrder::Reverse] while `false` maps to [SortOrder::Default].
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(sort) = &config.sorting {
-            if let Some(reverse) = sort.reverse {
-                if reverse {
-                    Some(Self::Reverse)
-                } else {
-                    Some(Self::Default)
-                }
-            } else {
-                None
-            }
-        } else {
-            None
-        }
+        config.sorting.as_ref().and_then(|s| match s.reverse {
+            Some(true) => Some(Self::Reverse),
+            Some(false) => Some(Self::Default),
+            None => None,
+        })
     }
 }
 
@@ -151,15 +137,14 @@ pub enum DirGrouping {
 }
 
 impl DirGrouping {
-    fn from_str(value: &str) -> Option<Self> {
+    fn from_arg_str(value: &str) -> Self {
         match value {
-            "first" => Some(Self::First),
-            "last" => Some(Self::Last),
-            "none" => Some(Self::None),
-            _ => panic!(
-                "Group Dir can only be one of first, last or none, but got {}.",
-                value
-            ),
+            "first" => Self::First,
+            "last" => Self::Last,
+            "none" => Self::None,
+            _ => {
+                unreachable!("Invalid value should be handled by `clap`");
+            }
         }
     }
 }
@@ -179,10 +164,12 @@ impl Configurable<Self> for DirGrouping {
         }
 
         if matches.occurrences_of("group-dirs") > 0 {
-            if let Some(group_dirs) = matches.values_of("group-dirs")?.last() {
-                return Self::from_str(group_dirs);
-            }
+            return matches
+                .values_of("group-dirs")?
+                .last()
+                .map(Self::from_arg_str);
         }
+
         None
     }
 
@@ -194,13 +181,11 @@ impl Configurable<Self> for DirGrouping {
     /// is one of "first", "last" or "none", this returns its corresponding variant in a [Some].
     /// Otherwise this returns [None].
     fn from_config(config: &Config) -> Option<Self> {
-        if let Some(true) = config.classic {
-            return Some(Self::None);
+        if config.classic == Some(true) {
+            Some(Self::None)
+        } else {
+            config.sorting.as_ref().and_then(|s| s.dir_grouping)
         }
-        if let Some(sort) = &config.sorting {
-            return sort.dir_grouping;
-        }
-        None
     }
 }
 
@@ -488,11 +473,9 @@ mod test_dir_grouping {
     use crate::flags::Configurable;
 
     #[test]
-    #[should_panic(
-        expected = "Group Dir can only be one of first, last or none, but got bad value."
-    )]
+    #[should_panic]
     fn test_from_str_bad_value() {
-        assert_eq!(None, DirGrouping::from_str("bad value"));
+        DirGrouping::from_arg_str("bad value");
     }
 
     #[test]