Repository: rust-itertools/itertools
Pull request #785 (2023-10-20 13:59:48)

GroupMap: add fold_with (2) more generic init
Hi, this completes #778 

@Philippe-Cholet Could you please approve this? Apparently I cannot simply push anymore to the repo.

@jswrenn I ran into this problem twice now. Am I doing something wrong? Is there a way to allow fixups like this for maintainers?

tamird: It seems a bit strange to accept the value since init is only called for the first key-value pair in a group. The value that the closure will be called with is not guaranteed to be deterministic AFAIK.
Philippe-Cholet: @tamird I agree it's weird to add the `value` parameter, I'm not fond of it. But I guess it's not a big pain to ignore it by adding `,_` to the closure. And maybe there is an usage for it.

@phimuemue Can't you approve them yourself? In the "Files changed" page: click "Review changes" button, check "Approve" box, valid by clicking "Submit review" button. That's what I would do. I'd like to know where you are blocked if you are.
phimuemue: @Philippe-Cholet I already tried this. "Approve" is disabled (seems I cannot review my own PRs).
phimuemue: > It seems a bit strange to accept the value since init is only called for the first key-value pair in a group. The value that the closure will be called with is not guaranteed to be deterministic AFAIK.

True, but `fold_with` is merely a shorthand that avoids `Option`s. `val` is deterministic: the iterator's first item associated with its key.

Patch:
diff --git a/src/grouping_map.rs b/src/grouping_map.rs
index 8799c6e..aeb86f1 100644
--- a/src/grouping_map.rs
+++ b/src/grouping_map.rs
@@ -138,7 +138,7 @@ where
     ///
     /// let lookup = (1..=7)
     ///     .into_grouping_map_by(|&n| n % 3)
-    ///     .fold_with(|_key| Default::default(), |Accumulator { acc }, _key, val| {
+    ///     .fold_with(|_key, _val| Default::default(), |Accumulator { acc }, _key, val| {
     ///         let acc = acc + val;
     ///         Accumulator { acc }
     ///      });
@@ -150,11 +150,11 @@ where
     /// ```
     pub fn fold_with<FI, FO, R>(self, mut init: FI, mut operation: FO) -> HashMap<K, R>
     where
-        FI: FnMut(&K) -> R,
+        FI: FnMut(&K, &V) -> R,
         FO: FnMut(R, &K, V) -> R,
     {
         self.aggregate(|acc, key, val| {
-            let acc = acc.unwrap_or_else(|| init(key));
+            let acc = acc.unwrap_or_else(|| init(key, &val));
             Some(operation(acc, key, val))
         })
     }
@@ -189,7 +189,7 @@ where
         R: Clone,
         FO: FnMut(R, &K, V) -> R,
     {
-        self.fold_with(|_: &K| init.clone(), operation)
+        self.fold_with(|_, _| init.clone(), operation)
     }
 
     /// Groups elements from the `GroupingMap` source by key and applies `operation` to the elements


Test patch:
diff --git a/tests/quick.rs b/tests/quick.rs
index d65e274..6f45a63 100644
--- a/tests/quick.rs
+++ b/tests/quick.rs
@@ -1481,7 +1481,7 @@ quickcheck! {
         let modulo = if modulo == 0 { 1 } else { modulo } as u64; // Avoid `% 0`
         let lookup = a.iter().map(|&b| b as u64) // Avoid overflows
             .into_grouping_map_by(|i| i % modulo)
-            .fold_with(|_key| Default::default(), |Accumulator { acc }, &key, val| {
+            .fold_with(|_key, _val| Default::default(), |Accumulator { acc }, &key, val| {
                 assert!(val % modulo == key);
                 let acc = acc + val;
                 Accumulator { acc }