From 9fa115500dc018ef47a6c2802789ac59d29dd49e Mon Sep 17 00:00:00 2001 From: Matthew Piziak Date: Tue, 11 Oct 2016 17:31:47 -0400 Subject: [PATCH] Simplify FoldResult implementation - Add tests for FoldResult - Make documentation more explicit - Assert failure accumulator in test Submitted-by: Matthew Piziak Signed-off-by: Matthias Beyer --- libimagentrylist/src/listers/line.rs | 2 +- libimagentrylist/src/listers/path.rs | 2 +- libimagrt/src/configuration.rs | 3 +- libimagstore/src/configuration.rs | 4 +- libimagstore/src/hook/aspect.rs | 6 +-- libimagstore/src/store.rs | 5 +-- libimagutil/src/iter.rs | 55 ++++++++++++++++++++-------- 7 files changed, 49 insertions(+), 28 deletions(-) diff --git a/libimagentrylist/src/listers/line.rs b/libimagentrylist/src/listers/line.rs index 66181542..8c439934 100644 --- a/libimagentrylist/src/listers/line.rs +++ b/libimagentrylist/src/listers/line.rs @@ -46,7 +46,7 @@ impl<'a> Lister for LineLister<'a> { use error::ListError as LE; use error::ListErrorKind as LEK; - entries.fold_defresult(|entry| { + entries.fold_result(|entry| { let s = entry.get_location().to_str().unwrap_or(String::from(self.unknown_output)); write!(stdout(), "{:?}\n", s).map_err(|e| LE::new(LEK::FormatError, Some(Box::new(e)))) }) diff --git a/libimagentrylist/src/listers/path.rs b/libimagentrylist/src/listers/path.rs index 284ff1b3..15a2df5b 100644 --- a/libimagentrylist/src/listers/path.rs +++ b/libimagentrylist/src/listers/path.rs @@ -47,7 +47,7 @@ impl Lister for PathLister { use error::ListError as LE; use error::ListErrorKind as LEK; - entries.fold_defresult(|entry| { + entries.fold_result(|entry| { Ok(entry.get_location().clone()) .and_then(|pb| pb.into_pathbuf().map_err_into(LEK::FormatError)) .and_then(|pb| { diff --git a/libimagrt/src/configuration.rs b/libimagrt/src/configuration.rs index 4601023e..63d24e15 100644 --- a/libimagrt/src/configuration.rs +++ b/libimagrt/src/configuration.rs @@ -147,7 +147,7 @@ impl Configuration { None => Err(CEK::ConfigOverrideKeyNotAvailable.into_error()), } }) - .fold_defresult(|i| i) + .fold_result(|i| i) .map_err(Box::new) .map_err(|e| CEK::ConfigOverrideError.into_error_with_cause(e)) } @@ -274,4 +274,3 @@ fn fetch_config(rtp: &PathBuf) -> Result { .map(|inner| Value::Table(inner.unwrap())) .ok_or(ConfigErrorKind::NoConfigFileFound.into()) } - diff --git a/libimagstore/src/configuration.rs b/libimagstore/src/configuration.rs index d8e08d44..fd73674b 100644 --- a/libimagstore/src/configuration.rs +++ b/libimagstore/src/configuration.rs @@ -87,7 +87,7 @@ pub fn config_is_valid(config: &Option) -> Result<()> { }) .and_then(|t| match *t { Value::Array(ref a) => { - a.iter().fold_defresult(|elem| if is_match!(*elem, Value::String(_)) { + a.iter().fold_result(|elem| if is_match!(*elem, Value::String(_)) { Ok(()) } else { let cause = Box::new(kind.into_error()); @@ -122,7 +122,7 @@ pub fn config_is_valid(config: &Option) -> Result<()> { }) .and_then(|section_table| match *section_table { // which is Value::Table(ref section_table) => // a table - section_table.iter().fold_defresult(|(inner_key, cfg)| { + section_table.iter().fold_result(|(inner_key, cfg)| { match *cfg { Value::Table(ref hook_config) => { // are tables // with a key diff --git a/libimagstore/src/hook/aspect.rs b/libimagstore/src/hook/aspect.rs index 40dc6085..75cd595f 100644 --- a/libimagstore/src/hook/aspect.rs +++ b/libimagstore/src/hook/aspect.rs @@ -73,7 +73,7 @@ impl StoreIdAccessor for Aspect { return Err(HE::new(HEK::AccessTypeViolation, None)); } - accessors.iter().fold_defresult(|accessor| { + accessors.iter().fold_result(|accessor| { let res = match accessor { &HDA::StoreIdAccess(accessor) => accessor.access(id), _ => unreachable!(), @@ -94,7 +94,7 @@ impl MutableHookDataAccessor for Aspect { // More sophisticated version would check whether there are _chunks_ of // NonMutableAccess accessors and execute these chunks in parallel. We do not have // performance concerns yet, so this is okay. - accessors.iter().fold_defresult(|accessor| { + accessors.iter().fold_result(|accessor| { let res = match accessor { &HDA::StoreIdAccess(ref accessor) => accessor.access(fle.get_location()), &HDA::NonMutableAccess(ref accessor) => accessor.access(fle), @@ -127,7 +127,7 @@ impl NonMutableHookDataAccessor for Aspect { return Err(HE::new(HEK::AccessTypeViolation, None)); } - accessors.iter().fold_defresult(|accessor| { + accessors.iter().fold_result(|accessor| { let res = match accessor { &HDA::NonMutableAccess(accessor) => accessor.access(fle), _ => unreachable!(), diff --git a/libimagstore/src/store.rs b/libimagstore/src/store.rs index 9bb1affc..27f6af51 100644 --- a/libimagstore/src/store.rs +++ b/libimagstore/src/store.rs @@ -1003,7 +1003,7 @@ impl Store { match aspects.lock() { Err(_) => return Err(HookErrorKind::HookExecutionError.into()), Ok(g) => g - }.iter().fold_defresult(|aspect| { + }.iter().fold_result(|aspect| { debug!("[Aspect][exec]: {:?}", aspect); (aspect as &StoreIdAccessor).access(id) }).map_err(Box::new) @@ -1025,7 +1025,7 @@ impl Store { match aspects.lock() { Err(_) => return Err(HookErrorKind::HookExecutionError.into()), Ok(g) => g - }.iter().fold_defresult(|aspect| { + }.iter().fold_result(|aspect| { debug!("[Aspect][exec]: {:?}", aspect); aspect.access_mut(fle) }).map_err(Box::new) @@ -2249,4 +2249,3 @@ aspect = "test" assert!(store.update(&mut fle).is_ok()); } } - diff --git a/libimagutil/src/iter.rs b/libimagutil/src/iter.rs index 2f2bb5b7..554ab72c 100644 --- a/libimagutil/src/iter.rs +++ b/libimagutil/src/iter.rs @@ -1,4 +1,3 @@ -// // imag - the personal information management suite for the commandline // Copyright (C) 2015, 2016 Matthias Beyer and contributors // @@ -21,29 +20,53 @@ pub trait FoldResult: Sized { type Item; - /// Processes all contained items returning the last successful result or the first error. - /// If there are no items, returns `Ok(R::default())`. - fn fold_defresult(self, func: F) -> Result - where R: Default, - F: FnMut(Self::Item) - -> Result - { - self.fold_result(R::default(), func) - } - - /// Processes all contained items returning the last successful result or the first error. - /// If there are no items, returns `Ok(default)`. - fn fold_result(self, default: R, mut func: F) -> Result + /// Apply a `FnMut(Self::Item) -> Result` to each item. If each + /// application returns an `Ok(_)`, return `Ok(())`, indicating success. + /// Otherwise return the first error. + /// + /// The return type of this function only indicates success with the + /// `Ok(())` idiom. To retrieve the values of your application, include an + /// accumulator in `func`. This is the intended reason for the permissive + /// `FnMut` type. + fn fold_result(self, mut func: F) -> Result<(), E> where F: FnMut(Self::Item) -> Result; } impl> FoldResult for I { type Item = X; - fn fold_result(self, default: R, mut func: F) -> Result + fn fold_result(self, mut func: F) -> Result<(), E> where F: FnMut(Self::Item) -> Result { - self.fold(Ok(default), |acc, item| acc.and_then(|_| func(item))) + for item in self { + try!(func(item)); + } + Ok(()) } } +#[test] +fn test_fold_result_success() { + let v = vec![1, 2, 3]; + let mut accum = vec![]; + let result: Result<(), &str> = v.iter().fold_result(|item| { + accum.push(*item * 2); + Ok(*item) + }); + assert_eq!(result, Ok(())); + assert_eq!(accum, vec![2, 4, 6]); +} + +#[test] +fn test_fold_result_failure() { + let v: Vec = vec![1, 2, 3]; + let mut accum: Vec = vec![]; + let result: Result<(), &str> = v.iter().fold_result(|item| if *item == 2 { + Err("failure") + } else { + accum.push(*item * 2); + Ok(*item) + }); + assert_eq!(result, Err("failure")); + assert_eq!(accum, vec![2]); +}