From 6b309d5cfca70f595495d2621f53f43b2c601b69 Mon Sep 17 00:00:00 2001 From: Benjamin Sago Date: Wed, 9 Aug 2017 19:18:31 +0100 Subject: [PATCH] Make SizeFormat lenient, and add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 80 ++++++++++++++++++++++++++----------------- src/options/view.rs | 26 ++++++++------ 2 files changed, 64 insertions(+), 42 deletions(-) diff --git a/src/options/parser.rs b/src/options/parser.rs index 882808f..04ea56e 100644 --- a/src/options/parser.rs +++ b/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 { - match self.strictness { - 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::>(); + self.has_where(|flag| flag.matches(arg)).map(|flag| flag.is_some()) + } - if all.len() < 2 { Ok(all.len() == 1) } - else { Err(Misfire::Conflict(arg, arg)) } - } + /// Returns the first found argument that satisfies the predicate, or + /// 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

(&self, predicate: P) -> Result, 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::>(); + + 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, Misfire> { self.get_where(|flag| flag.matches(arg)) } - /// If the given argument was specified, return its value. - /// The value is not guaranteed to be valid UTF-8. + /// Returns the value of the argument that matches the predicate if it + /// 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

(&self, predicate: P) -> Result, Misfire> where P: Fn(&Flag) -> bool { - match self.strictness { - UseLastArguments => { - let found = self.flags.iter().rev() - .find(|tuple| tuple.1.is_some() && predicate(&tuple.0)) - .map(|tuple| tuple.1.unwrap()); - Ok(found) - } - ComplainAboutRedundantArguments => { - let those = self.flags.iter() - .filter(|tuple| tuple.1.is_some() && predicate(&tuple.0)) - .collect::>(); + if self.is_strict() { + let those = self.flags.iter() + .filter(|tuple| tuple.1.is_some() && predicate(&tuple.0)) + .collect::>(); - 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())) } - } + 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 { + 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)) diff --git a/src/options/view.rs b/src/options/view.rs index b905c17..b03adb0 100644 --- a/src/options/view.rs +++ b/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 { - let binary = matches.has(&flags::BINARY)?; - let bytes = matches.has(&flags::BYTES)?; + let flag = matches.has_where(|f| f.matches(&flags::BINARY) || f.matches(&flags::BYTES))?; - match (binary, bytes) { - (true, true ) => Err(Misfire::Conflict(&flags::BINARY, &flags::BYTES)), - (true, false) => Ok(SizeFormat::BinaryBytes), - (false, true ) => Ok(SizeFormat::JustBytes), - (false, false) => Ok(SizeFormat::DecimalBytes), - } + Ok(match flag { + Some(f) if f.matches(&flags::BINARY) => SizeFormat::BinaryBytes, + Some(f) if f.matches(&flags::BYTES) => SizeFormat::JustBytes, + _ => 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 - test!(both: SizeFormat <- ["--binary", "--bytes"]; Both => Err(Misfire::Conflict(&flags::BINARY, &flags::BYTES))); + // Overriding + 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")))); }