From 685e949545036bd67c936d930b268ce6dea7ba44 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ionu=C8=9B=20G=2E=20Stan?= Date: Wed, 11 Oct 2017 19:31:06 +0300 Subject: [PATCH 1/5] Require an Order instance for NonEmptyList's groupBy function This addresses: #1959 --- core/src/main/scala/cats/data/NonEmptyList.scala | 10 +++++----- core/src/main/scala/cats/syntax/list.scala | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/core/src/main/scala/cats/data/NonEmptyList.scala b/core/src/main/scala/cats/data/NonEmptyList.scala index 875609c236..ee66a9e11f 100644 --- a/core/src/main/scala/cats/data/NonEmptyList.scala +++ b/core/src/main/scala/cats/data/NonEmptyList.scala @@ -325,20 +325,20 @@ 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]] = { + val m = mutable.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]] + val b = immutable.TreeMap.newBuilder[B, NonEmptyList[A]](B.toOrdering) 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))) + b += k -> NonEmptyList.fromListUnsafe(v.result) } b.result } diff --git a/core/src/main/scala/cats/syntax/list.scala b/core/src/main/scala/cats/syntax/list.scala index d354eb7c7c..f8f21cf445 100644 --- a/core/src/main/scala/cats/syntax/list.scala +++ b/core/src/main/scala/cats/syntax/list.scala @@ -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]] = toNel.fold(Map.empty[B, NonEmptyList[A]])(_.groupBy(f)) } From 24ff9263b1cd8a2b6e796c69ad6c79082fc611f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ionu=C8=9B=20G=2E=20Stan?= Date: Thu, 12 Oct 2017 14:28:59 +0300 Subject: [PATCH 2/5] There's no mutable TreeMap before Scala 2.12; use only immutable --- .../main/scala/cats/data/NonEmptyList.scala | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/core/src/main/scala/cats/data/NonEmptyList.scala b/core/src/main/scala/cats/data/NonEmptyList.scala index ee66a9e11f..a7c1db265d 100644 --- a/core/src/main/scala/cats/data/NonEmptyList.scala +++ b/core/src/main/scala/cats/data/NonEmptyList.scala @@ -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 @@ -332,17 +332,19 @@ final case class NonEmptyList[+A](head: A, tail: List[A]) { * }}} */ def groupBy[B](f: A => B)(implicit B: Order[B]): Map[B, NonEmptyList[A]] = { - val m = mutable.TreeMap.empty[B, mutable.Builder[A, List[A]]](B.toOrdering) + 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.TreeMap.newBuilder[B, NonEmptyList[A]](B.toOrdering) - for { (k, v) <- m } { - b += k -> NonEmptyList.fromListUnsafe(v.result) + val k = f(elem) + + m.get(k) match { + case None => m += ((k, List.newBuilder[A] += elem)) + case Some(builder) => builder += elem + } } - b.result - } + m.mapValues(v => NonEmptyList.fromListUnsafe(v.result)) + } } object NonEmptyList extends NonEmptyListInstances { From 9f116d61faea0987a56c2077d2b1b2440683bf7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ionu=C8=9B=20G=2E=20Stan?= Date: Thu, 12 Oct 2017 16:14:20 +0300 Subject: [PATCH 3/5] Don't use `mapValues`, as per code review comments https://github.com/typelevel/cats/pull/1964 --- core/src/main/scala/cats/data/NonEmptyList.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/cats/data/NonEmptyList.scala b/core/src/main/scala/cats/data/NonEmptyList.scala index a7c1db265d..45a88231d0 100644 --- a/core/src/main/scala/cats/data/NonEmptyList.scala +++ b/core/src/main/scala/cats/data/NonEmptyList.scala @@ -343,7 +343,7 @@ final case class NonEmptyList[+A](head: A, tail: List[A]) { } } - m.mapValues(v => NonEmptyList.fromListUnsafe(v.result)) + m.map { case (k, v) => (k, NonEmptyList.fromListUnsafe(v.result)) } } } From cace36c0f65a09f080d929db27a88a39068619e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ionu=C8=9B=20G=2E=20Stan?= Date: Thu, 12 Oct 2017 22:07:37 +0300 Subject: [PATCH 4/5] Have NonEmptyList.groupBy return a SortedMap instead of just a Map --- .../main/scala/cats/data/NonEmptyList.scala | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/core/src/main/scala/cats/data/NonEmptyList.scala b/core/src/main/scala/cats/data/NonEmptyList.scala index 45a88231d0..4dfc37dc61 100644 --- a/core/src/main/scala/cats/data/NonEmptyList.scala +++ b/core/src/main/scala/cats/data/NonEmptyList.scala @@ -3,9 +3,8 @@ package data import cats.instances.list._ import cats.syntax.order._ - import scala.annotation.tailrec -import scala.collection.immutable.{ TreeMap, TreeSet } +import scala.collection.immutable.{ SortedMap, TreeMap, TreeSet } import scala.collection.mutable import scala.collection.mutable.ListBuffer @@ -321,18 +320,21 @@ final case class NonEmptyList[+A](head: A, tail: List[A]) { } /** - * Groups elements inside of this `NonEmptyList` using a mapping function + * Groups elements inside this `NonEmptyList` according to the `Order` + * of the keys produced by the given mapping function. * * {{{ + * scala> import scala.collection.immutable.SortedMap * 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)) + * res0: SortedMap[Boolean, cats.data.NonEmptyList[Int]] = Map(false -> NonEmptyList(-2, -5), true -> NonEmptyList(12, 3)) * }}} */ - 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) + def groupBy[B](f: A => B)(implicit B: Order[B]): SortedMap[B, NonEmptyList[A]] = { + implicit val ordering: Ordering[B] = B.toOrdering + var m = TreeMap.empty[B, mutable.Builder[A, List[A]]] for { elem <- toList } { val k = f(elem) @@ -343,7 +345,9 @@ final case class NonEmptyList[+A](head: A, tail: List[A]) { } } - m.map { case (k, v) => (k, NonEmptyList.fromListUnsafe(v.result)) } + m.map { + case (k, v) => (k, NonEmptyList.fromListUnsafe(v.result)) + } : TreeMap[B, NonEmptyList[A]] } } From 2f0a1653bf28a4debadae9ddd1ccf80629a418b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ionu=C8=9B=20G=2E=20Stan?= Date: Fri, 13 Oct 2017 01:21:48 +0300 Subject: [PATCH 5/5] Have List.groupByNel return a SortedMap as well --- core/src/main/scala/cats/NonEmptyTraverse.scala | 3 +-- core/src/main/scala/cats/syntax/list.scala | 7 +++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/core/src/main/scala/cats/NonEmptyTraverse.scala b/core/src/main/scala/cats/NonEmptyTraverse.scala index cc7516269d..64527ef97c 100644 --- a/core/src/main/scala/cats/NonEmptyTraverse.scala +++ b/core/src/main/scala/cats/NonEmptyTraverse.scala @@ -53,9 +53,8 @@ import simulacrum.typeclass * {{{ * scala> import cats.implicits._ * scala> import cats.data.NonEmptyList - * scala> def countWords(words: List[String]): Map[String, Int] = words.groupBy(identity).mapValues(_.length) * scala> val x = NonEmptyList.of(List("How", "do", "you", "fly"), List("What", "do", "you", "do")) - * scala> x.nonEmptyFlatTraverse(_.groupByNel(identity)) + * scala> x.nonEmptyFlatTraverse(_.groupByNel(identity) : Map[String, NonEmptyList[String]]) * res0: Map[String,cats.data.NonEmptyList[String]] = Map(do -> NonEmptyList(do, do, do), you -> NonEmptyList(you, you)) * }}} */ diff --git a/core/src/main/scala/cats/syntax/list.scala b/core/src/main/scala/cats/syntax/list.scala index f8f21cf445..b8e31dc83e 100644 --- a/core/src/main/scala/cats/syntax/list.scala +++ b/core/src/main/scala/cats/syntax/list.scala @@ -1,6 +1,7 @@ package cats package syntax +import scala.collection.immutable.SortedMap import cats.data.NonEmptyList trait ListSyntax { @@ -27,6 +28,8 @@ final class ListOps[A](val la: List[A]) extends AnyVal { * }}} */ def toNel: Option[NonEmptyList[A]] = NonEmptyList.fromList(la) - def groupByNel[B : Order](f: A => B): Map[B, NonEmptyList[A]] = - toNel.fold(Map.empty[B, NonEmptyList[A]])(_.groupBy(f)) + def groupByNel[B](f: A => B)(implicit B: Order[B]): SortedMap[B, NonEmptyList[A]] = { + implicit val ordering = B.toOrdering + toNel.fold(SortedMap.empty[B, NonEmptyList[A]])(_.groupBy(f)) + } }