Simplify FoldResult implementation

- Add tests for FoldResult
- Make documentation more explicit
- Assert failure accumulator in test

Submitted-by: Matthew Piziak <matthew.piziak@gmail.com>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>

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))))
         })

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| {

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<Value> {
         .map(|inner| Value::Table(inner.unwrap()))
         .ok_or(ConfigErrorKind::NoConfigFileFound.into())
 }
-

libimagstore/src/configuration.rs

@@ -87,7 +87,7 @@ pub fn config_is_valid(config: &Option<Value>) -> 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<Value>) -> 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

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!(),

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());
     }
 }
-

libimagutil/src/iter.rs

@@ -1,4 +1,3 @@
-//
 // imag - the personal information management suite for the commandline
 // Copyright (C) 2015, 2016 Matthias Beyer <mail@beyermatthias.de> 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<R, E, F>(self, func: F) -> Result<R, E>
-        where R: Default,
-              F: FnMut(Self::Item)
-        -> Result<R, E>
-    {
-        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<R, E, F>(self, default: R, mut func: F) -> Result<R, E>
+    /// Apply a `FnMut(Self::Item) -> Result<R, E>` 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<R, E, F>(self, mut func: F) -> Result<(), E>
         where F: FnMut(Self::Item) -> Result<R, E>;
 }
 
 impl<X, I: Iterator<Item = X>> FoldResult for I {
     type Item = X;
 
-    fn fold_result<R, E, F>(self, default: R, mut func: F) -> Result<R, E>
+    fn fold_result<R, E, F>(self, mut func: F) -> Result<(), E>
         where F: FnMut(Self::Item) -> Result<R, E>
     {
-        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<usize> = vec![1, 2, 3];
+    let mut accum: Vec<usize> = 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]);
+}