-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Require an Order instance for NonEmptyList's groupBy function #1964
Changes from 3 commits
685e949
24ff926
9f116d6
cace36c
2f0a165
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,9 +5,9 @@ import cats.instances.list._ | |
import cats.syntax.order._ | ||
|
||
import scala.annotation.tailrec | ||
import scala.collection.immutable.TreeSet | ||
import scala.collection.immutable.{ TreeMap, TreeSet } | ||
import scala.collection.mutable | ||
import scala.collection.mutable.ListBuffer | ||
import scala.collection.{immutable, mutable} | ||
|
||
/** | ||
* A data type which represents a non empty list of A, with | ||
|
@@ -325,24 +325,26 @@ final case class NonEmptyList[+A](head: A, tail: List[A]) { | |
* | ||
* {{{ | ||
* scala> import cats.data.NonEmptyList | ||
* scala> import cats.instances.boolean._ | ||
* scala> val nel = NonEmptyList.of(12, -2, 3, -5) | ||
* scala> nel.groupBy(_ >= 0) | ||
* res0: Map[Boolean, cats.data.NonEmptyList[Int]] = Map(false -> NonEmptyList(-2, -5), true -> NonEmptyList(12, 3)) | ||
* }}} | ||
*/ | ||
def groupBy[B](f: A => B): Map[B, NonEmptyList[A]] = { | ||
val m = mutable.Map.empty[B, mutable.Builder[A, List[A]]] | ||
def groupBy[B](f: A => B)(implicit B: Order[B]): Map[B, NonEmptyList[A]] = { | ||
var m = TreeMap.empty[B, mutable.Builder[A, List[A]]](B.toOrdering) | ||
|
||
for { elem <- toList } { | ||
m.getOrElseUpdate(f(elem), List.newBuilder[A]) += elem | ||
} | ||
val b = immutable.Map.newBuilder[B, NonEmptyList[A]] | ||
for { (k, v) <- m } { | ||
val head :: tail = v.result // we only create non empty list inside of the map `m` | ||
b += ((k, NonEmptyList(head, tail))) | ||
val k = f(elem) | ||
|
||
m.get(k) match { | ||
case None => m += ((k, List.newBuilder[A] += elem)) | ||
case Some(builder) => builder += elem | ||
} | ||
} | ||
b.result | ||
} | ||
|
||
m.map { case (k, v) => (k, NonEmptyList.fromListUnsafe(v.result)) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we ascribe a type to this to make sure we are not converting to using hashCode and equals at the end? m.map { case (k, v) => (k, NonEmptyList.fromListUnsafe(v.result)) } : TreeMap[B, NonEmptyList[A]] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. One further thought. Shall we also change the return type to |
||
} | ||
} | ||
|
||
object NonEmptyList extends NonEmptyListInstances { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,6 @@ final class ListOps[A](val la: List[A]) extends AnyVal { | |
* }}} | ||
*/ | ||
def toNel: Option[NonEmptyList[A]] = NonEmptyList.fromList(la) | ||
def groupByNel[B](f: A => B): Map[B, NonEmptyList[A]] = | ||
def groupByNel[B : Order](f: A => B): Map[B, NonEmptyList[A]] = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't the type here also be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it should. Working on it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well... current status: yak-shaving inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've pushed something that addresses this. I've fixed the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think making an Apply and Traversable for SortedMap is probably out of scope, so I think that's okay. |
||
toNel.fold(Map.empty[B, NonEmptyList[A]])(_.groupBy(f)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add to the scaladoc a brief description of why
Order
is required?