Add groupMap family of methods to NonEmptyVector and NonEmptySeq#4826
Add groupMap family of methods to NonEmptyVector and NonEmptySeq#4826arnavsharma990 wants to merge 1 commit intotypelevel:mainfrom
Conversation
…uceWith/groupMapReduceWithNem to NonEmptyVector and NonEmptySeq NonEmptyList and NonEmptyChain both expose a rich groupMap family of methods: - groupMap / groupMapNem - groupMapReduce / groupMapReduceNem - groupMapReduceWith / groupMapReduceWithNem NonEmptyVector and NonEmptySeq only had groupBy / groupByNem, creating an inconsistency across the four primary NonEmpty collection types. This commit brings NonEmptyVector and NonEmptySeq up to parity by adding all six missing methods to each, following the same implementation pattern used in NonEmptyList (explicit TreeMap accumulation loop) and the same scaladoc style used throughout the codebase.
There was a problem hiding this comment.
Pull request overview
This PR fills an API gap by adding the full groupMap family of grouping/transformation/reduction methods to NonEmptyVector and NonEmptySeq, bringing them in line with NonEmptyList and NonEmptyChain.
Changes:
- Add
groupMap/groupMapNemtoNonEmptyVectorandNonEmptySeq. - Add
groupMapReduce/groupMapReduceNemtoNonEmptyVectorandNonEmptySeq. - Add
groupMapReduceWith/groupMapReduceWithNemtoNonEmptyVectorandNonEmptySeq, following existing TreeMap accumulation patterns and providing scaladoc examples.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| core/src/main/scala/cats/data/NonEmptyVector.scala | Adds the groupMap* method family returning SortedMap/NonEmptyMap with NonEmptyVector group values. |
| core/src/main/scala/cats/data/NonEmptySeq.scala | Adds the groupMap* method family returning SortedMap/NonEmptyMap with NonEmptySeq group values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| final def groupBy[B](f: A => B)(implicit B: Order[B]): SortedMap[B, NonEmptyVector[A]] = { | ||
| implicit val ordering: Ordering[B] = B.toOrdering | ||
| var m = TreeMap.empty[B, mutable.Builder[A, Vector[A]]] | ||
|
|
||
| for { elem <- toVector } { | ||
| val k = f(elem) | ||
|
|
There was a problem hiding this comment.
Now that groupMap exists, groupBy duplicates almost the same TreeMap/builder accumulation logic. Consider re-implementing groupBy as groupMap(key = f)(identity) to reduce duplication and ensure future changes to the grouping algorithm stay consistent across both methods (similar to NonEmptyList’s groupBy delegating to groupMap).
| final def groupMap[K, B](key: A => K)(f: A => B)(implicit K: Order[K]): SortedMap[K, NonEmptyVector[B]] = { | ||
| implicit val ordering: Ordering[K] = K.toOrdering | ||
| var m = TreeMap.empty[K, mutable.Builder[B, Vector[B]]] | ||
|
|
||
| for { elem <- toVector } { | ||
| val k = key(elem) | ||
|
|
||
| m.get(k) match { | ||
| case None => m += ((k, Vector.newBuilder[B] += f(elem))) | ||
| case Some(builder) => builder += f(elem) | ||
| } | ||
| } | ||
|
|
||
| m.map { case (k, v) => | ||
| (k, NonEmptyVector.fromVectorUnsafe(v.result())) | ||
| }: TreeMap[K, NonEmptyVector[B]] | ||
| } |
There was a problem hiding this comment.
The new groupMap* methods add non-trivial behavior but there are currently no unit/property tests covering them for NonEmptyVector (unlike NonEmptyList, which has explicit groupMap/groupMapReduce* tests). Please add tests in NonEmptyVectorSuite covering groupMap, groupMapNem, groupMapReduce, groupMapReduceNem, groupMapReduceWith, and groupMapReduceWithNem (e.g., consistency with toVector.toList.groupBy + map/reduce).
| final def groupBy[B](f: A => B)(implicit B: Order[B]): SortedMap[B, NonEmptySeq[A]] = { | ||
| implicit val ordering: Ordering[B] = B.toOrdering | ||
| var m = TreeMap.empty[B, mutable.Builder[A, Seq[A]]] | ||
|
|
||
| for { elem <- toSeq } { | ||
| val k = f(elem) | ||
|
|
There was a problem hiding this comment.
Now that groupMap exists, groupBy duplicates almost the same TreeMap/builder accumulation logic. Consider re-implementing groupBy as groupMap(key = f)(identity) to reduce duplication and keep the grouping algorithm consistent across both methods (similar to how NonEmptyList’s groupBy delegates to groupMap).
| final def groupMap[K, B](key: A => K)(f: A => B)(implicit K: Order[K]): SortedMap[K, NonEmptySeq[B]] = { | ||
| implicit val ordering: Ordering[K] = K.toOrdering | ||
| var m = TreeMap.empty[K, mutable.Builder[B, Seq[B]]] | ||
|
|
||
| for { elem <- toSeq } { | ||
| val k = key(elem) | ||
|
|
||
| m.get(k) match { | ||
| case None => m += ((k, Seq.newBuilder[B] += f(elem))) | ||
| case Some(builder) => builder += f(elem) | ||
| } | ||
| } | ||
|
|
||
| m.map { case (k, v) => | ||
| (k, NonEmptySeq.fromSeqUnsafe(v.result())) | ||
| }: TreeMap[K, NonEmptySeq[B]] | ||
| } |
There was a problem hiding this comment.
The new groupMap* methods add non-trivial behavior but there are currently no unit/property tests covering them for NonEmptySeq (unlike NonEmptyList, which has explicit groupMap/groupMapReduce* tests). Please add tests in NonEmptySeqSuite covering groupMap, groupMapNem, groupMapReduce, groupMapReduceNem, groupMapReduceWith, and groupMapReduceWithNem (e.g., consistency with toSeq.toList.groupBy + map/reduce).
Motivation
NonEmptyListandNonEmptyChainboth have a full set ofgroupMapmethods available:groupMapSortedMap[K, NonEmptyList[B]]groupMapNemNonEmptyMap[K, NonEmptyList[B]]groupMapReduceSortedMap[K, B]groupMapReduceNemNonEmptyMap[K, B]groupMapReduceWithSortedMap[K, B]groupMapReduceWithNemNonEmptyMap[K, B]NonEmptyVectorandNonEmptySeqonly hadgroupBy/groupByNem, leading to an inconsistency in the API between the four mainNonEmpty*collection types.Changes
Adds all six missing methods to both
NonEmptyVectorandNonEmptySeq:groupMap/groupMapNem- group and transform elements, returningNonEmptyVector[B]/NonEmptySeq[B]per groupgroupMapReduce/groupMapReduceNem- group and transform and reduce usingSemigroupgroupMapReduceWith/groupMapReduceWithNem- group and transform and reduce using explicit combine function