Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NonEmptyList.groupByNelA #3432

Merged
merged 12 commits into from
Jun 7, 2020
Merged

NonEmptyList.groupByNelA #3432

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5c64869
adding groupByNelA implementation
pk044 May 25, 2020
3bb21f8
adding groupByNelA documentation
pk044 May 25, 2020
6bc955d
adding groupByNelA test for consistency with groupByNel
pk044 May 25, 2020
fccaa58
Merge branch 'master' of https://github.com/typelevel/cats into group…
pk044 May 25, 2020
3d9b795
formatting
pk044 May 25, 2020
4ca92bb
use Functor[SortedMap[B, ?]].map instead of mapValues
pk044 May 26, 2020
026b5b3
replacing `?` with `*`
pk044 May 26, 2020
14a0f3d
more readable documentation
pk044 May 26, 2020
c131bf8
reordered implicits
pk044 May 27, 2020
f0f5b8f
functor instance moved out of fold
pk044 May 27, 2020
0f6aadd
formatting
May 27, 2020
4a67159
fixed doctest
May 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion core/src/main/scala/cats/syntax/list.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package cats
package syntax

import cats.data.{NonEmptyChain, NonEmptyList}

import scala.collection.immutable.SortedMap

trait ListSyntax {
Expand Down Expand Up @@ -52,6 +51,33 @@ final class ListOps[A](private val la: List[A]) extends AnyVal {
toNel.fold(SortedMap.empty[B, NonEmptyList[A]])(_.groupBy(f))
}

/**
* Groups elements inside this `List` according to the `Order` of the keys
* produced by the given mapping monadic function.
*
* {{{
* scala> import cats.data.NonEmptyList
* scala> import scala.collection.immutable.SortedMap
* scala> import cats.implicits._
*
* scala> val list = List(12, -2, 3, -5)
*
* scala> val expectedResult = Option(SortedMap(false -> NonEmptyList.of(-2, -5), true -> NonEmptyList.of(12, 3)))
*
* scala> list.groupByNelA(num => Option(0).map(num >= _)) === expectedResult
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw you don't need to do any explicit assertions here, you could just do

scala> list.groupByNelA(num => Option(0).map(num >= _))
res0: Option(SortedMap(false -> NonEmptyList(-2, -5), true -> NonEmptyList(12, 3)))

which will be a little less noise for the reader, while still asserting that the resulting expression is the same as what you typed. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, this one is pretty interesting

[info] > Labels of failing property: 

[info] 'Some(TreeMap(false -> NonEmptyList(-2, -5), true -> NonEmptyList(12, 3)))' is not equal to 'Some(Map(false -> NonEmptyList(-2, -5), true -> NonEmptyList(12, 3)))'

this seems to fail only on Scala 2.13.1 🤔 works for me locally and on the rest of Scala versions https://travis-ci.org/github/typelevel/cats/builds/691476028

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for you locally on 2.13?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.12.10

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quite not sure how to approach this - is there anything I can do to work it out or should I just stay with the explicit assertion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an issue with toString changing for some collection types between 2.12 and 2.13. Unfortunately I think you have to do something like this: #3155

* res0: Boolean = true
* }}}
*/
def groupByNelA[F[_], B](f: A => F[B])(implicit B: Order[B], F: Applicative[F]): F[SortedMap[B, NonEmptyList[A]]] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to order the implicit parameters this way? There are a couple of rules of thumb that would suggest reversing them (follow the declaration order of the type parameters and put instances that characterize type constructors first).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SIde note: I just took a closer look at this, and Cats is currently inconsistent in this respect—there are a few methods like WriterT.liftF that use the Monoid[L], Applicative[F] order for no obvious reason, but in general the codebase follows the other order, and I think we should here (unless of course there's a specific reason you did it this way).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, done!

implicit val ordering: Ordering[B] = B.toOrdering

toNel.fold(F.pure(SortedMap.empty[B, NonEmptyList[A]]))(nel =>
F.map(nel.traverse(a => F.tupleLeft(f(a), a)))(list =>
Functor[SortedMap[B, *]].map(list.groupBy(_._2))(_.map(_._1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't generally be a big deal, I guess, but instantiating this instance repeatedly doesn't seem ideal, and it'd be easy to lift it out.

)
)
}

/** Produces a `NonEmptyList` containing cumulative results of applying the
* operator going left to right.
*
Expand Down
6 changes: 6 additions & 0 deletions tests/src/test/scala/cats/tests/ListSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ class ListSuite extends CatsSuite {
}
)

test("groupByNelA should be consistent with groupByNel")(
forAll { (fa: List[Int], f: Int => Int) =>
fa.groupByNelA(f.andThen(Option(_))) should ===(Option(fa.groupByNel(f)))
}
)

test("show") {
List(1, 2, 3).show should ===("List(1, 2, 3)")
(Nil: List[Int]).show should ===("List()")
Expand Down