Make SizeFormat lenient, and add tests

This changes the SizeFormat option parser from its old, strict-by-default behaviour (where passing both --bytes and --binary would be an error) to the new, use-the-last-argument behaviour (where passing --bytes --binary would use --binary because it came later).

Doing this meant adding functionality to Matches so that it could return *which* argument matched. Previously, the order of --bytes and --binary didn’t matter, because they couldn’t both be present, but now it does.

src/options/parser.rs

@@ -314,61 +314,77 @@ pub struct MatchedFlags<'args> {
     strictness: Strictness,
 }
 
-use self::Strictness::*;
 impl<'a> MatchedFlags<'a> {
 
     /// Whether the given argument was specified.
+    /// Returns `true` if it was, `false` if it wasn’t, and an error in
+    /// strict mode if it was specified more than once.
     pub fn has(&self, arg: &'static Arg) -> Result<bool, Misfire> {
-        match self.strictness {
+        self.has_where(|flag| flag.matches(arg)).map(|flag| flag.is_some())
-            UseLastArguments => {
+    }
-                let any = self.flags.iter().rev()
-                              .find(|tuple| tuple.1.is_none() && tuple.0.matches(arg))
-                              .is_some();
-                Ok(any)
-            }
-            ComplainAboutRedundantArguments => {
-                let all = self.flags.iter()
-                              .filter(|tuple| tuple.1.is_none() && tuple.0.matches(arg))
-                              .collect::<Vec<_>>();
 
-                if all.len() < 2 { Ok(all.len() == 1) }
+    /// Returns the first found argument that satisfies the predicate, or
-                             else { Err(Misfire::Conflict(arg, arg)) }
+    /// nothing if none is found, or an error in strict mode if multiple
-            }
+    /// argument satisfy the predicate.
+    ///
+    /// You’ll have to test the resulting flag to see which argument it was.
+    pub fn has_where<P>(&self, predicate: P) -> Result<Option<&Flag>, Misfire>
+    where P: Fn(&Flag) -> bool {
+        if self.is_strict() {
+            let all = self.flags.iter()
+                          .filter(|tuple| tuple.1.is_none() && predicate(&tuple.0))
+                          .collect::<Vec<_>>();
+
+            if all.len() < 2 { Ok(all.first().map(|t| &t.0)) }
+                        else { Err(Misfire::Duplicate(all[0].0.clone(), all[1].0.clone())) }
+        }
+        else {
+            let any = self.flags.iter().rev()
+                          .find(|tuple| tuple.1.is_none() && predicate(&tuple.0))
+                          .map(|tuple| &tuple.0);
+            Ok(any)
         }
     }
 
     // This code could probably be better.
+    // Both ‘has’ and ‘get’ immediately begin with a conditional, which makes
+    // me think the functionality could be moved to inside Strictness.
 
+    /// Returns the value of the given argument if it was specified, nothing
+    /// if it wasn’t, and an error in strict mode if it was specified more
+    /// than once.
     pub fn get(&self, arg: &'static Arg) -> Result<Option<&OsStr>, Misfire> {
         self.get_where(|flag| flag.matches(arg))
     }
 
-    /// If the given argument was specified, return its value.
+    /// Returns the value of the argument that matches the predicate if it
-    /// The value is not guaranteed to be valid UTF-8.
+    /// was specified, nothing if it wasn't, and an error in strict mode if
+    /// multiple arguments matched the predicate.
+    ///
+    /// It’s not possible to tell which flag the value belonged to from this.
     pub fn get_where<P>(&self, predicate: P) -> Result<Option<&OsStr>, Misfire>
     where P: Fn(&Flag) -> bool {
-        match self.strictness {
+        if self.is_strict() {
-            UseLastArguments => {
+            let those = self.flags.iter()
-                let found = self.flags.iter().rev()
+                            .filter(|tuple| tuple.1.is_some() && predicate(&tuple.0))
-                                .find(|tuple| tuple.1.is_some() && predicate(&tuple.0))
+                            .collect::<Vec<_>>();
-                                .map(|tuple| tuple.1.unwrap());
-                Ok(found)
-            }
-            ComplainAboutRedundantArguments => {
-                let those = self.flags.iter()
-                                .filter(|tuple| tuple.1.is_some() && predicate(&tuple.0))
-                                .collect::<Vec<_>>();
 
-                if those.len() < 2 { Ok(those.first().cloned().map(|t| t.1.unwrap())) }
+            if those.len() < 2 { Ok(those.first().cloned().map(|t| t.1.unwrap())) }
-                               else { Err(Misfire::Duplicate(those[0].0.clone(), those[1].0.clone())) }
+                          else { Err(Misfire::Duplicate(those[0].0.clone(), those[1].0.clone())) }
-            }
+        }
+        else {
+            let found = self.flags.iter().rev()
+                            .find(|tuple| tuple.1.is_some() && predicate(&tuple.0))
+                            .map(|tuple| tuple.1.unwrap());
+            Ok(found)
         }
     }
 
     // It’s annoying that ‘has’ and ‘get’ won’t work when accidentally given
     // flags that do/don’t take values, but this should be caught by tests.
 
-    /// Counts the number of occurrences of the given argument.
+    /// Counts the number of occurrences of the given argument, even in
+    /// strict mode.
     pub fn count(&self, arg: &Arg) -> usize {
         self.flags.iter()
             .filter(|tuple| tuple.0.matches(arg))

src/options/view.rs

@@ -206,15 +206,13 @@ impl SizeFormat {
     /// involves the `--binary` or `--bytes` flags, and these conflict with
     /// each other.
     fn deduce(matches: &MatchedFlags) -> Result<SizeFormat, Misfire> {
-        let binary = matches.has(&flags::BINARY)?;
+        let flag = matches.has_where(|f| f.matches(&flags::BINARY) || f.matches(&flags::BYTES))?;
-        let bytes  = matches.has(&flags::BYTES)?;
 
-        match (binary, bytes) {
+        Ok(match flag {
-            (true,  true )  => Err(Misfire::Conflict(&flags::BINARY, &flags::BYTES)),
+            Some(f) if f.matches(&flags::BINARY)  => SizeFormat::BinaryBytes,
-            (true,  false)  => Ok(SizeFormat::BinaryBytes),
+            Some(f) if f.matches(&flags::BYTES)   => SizeFormat::JustBytes,
-            (false, true )  => Ok(SizeFormat::JustBytes),
+            _                                     => SizeFormat::DecimalBytes,
-            (false, false)  => Ok(SizeFormat::DecimalBytes),
+        })
-        }
     }
 }
 
@@ -463,8 +461,16 @@ mod test {
         test!(binary:  SizeFormat <- ["--binary"];             Both => Ok(SizeFormat::BinaryBytes));
         test!(bytes:   SizeFormat <- ["--bytes"];              Both => Ok(SizeFormat::JustBytes));
 
-        // Errors
+        // Overriding
-        test!(both:    SizeFormat <- ["--binary", "--bytes"];  Both => Err(Misfire::Conflict(&flags::BINARY, &flags::BYTES)));
+        test!(both_1:  SizeFormat <- ["--binary", "--binary"];  Last => Ok(SizeFormat::BinaryBytes));
+        test!(both_2:  SizeFormat <- ["--bytes",  "--binary"];  Last => Ok(SizeFormat::BinaryBytes));
+        test!(both_3:  SizeFormat <- ["--binary", "--bytes"];   Last => Ok(SizeFormat::JustBytes));
+        test!(both_4:  SizeFormat <- ["--bytes",  "--bytes"];   Last => Ok(SizeFormat::JustBytes));
+
+        test!(both_5:  SizeFormat <- ["--binary", "--binary"];  Complain => Err(Misfire::Duplicate(Flag::Long("binary"), Flag::Long("binary"))));
+        test!(both_6:  SizeFormat <- ["--bytes",  "--binary"];  Complain => Err(Misfire::Duplicate(Flag::Long("bytes"),  Flag::Long("binary"))));
+        test!(both_7:  SizeFormat <- ["--binary", "--bytes"];   Complain => Err(Misfire::Duplicate(Flag::Long("binary"), Flag::Long("bytes"))));
+        test!(both_8:  SizeFormat <- ["--bytes",  "--bytes"];   Complain => Err(Misfire::Duplicate(Flag::Long("bytes"),  Flag::Long("bytes"))));
     }