NonEmptyList.groupByNelA #3432
NonEmptyList.groupByNelA #3432
Conversation
| 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)))(_.groupBy(_._2).mapValues(_.map(_._1))) |
There was a problem hiding this comment.
mapValues was changed in the 2.13 collection rewrite, we could use its Functor instance here instead :)
There was a problem hiding this comment.
thank you for the suggestion! i'll push a fix soon :)
Codecov Report
@@ Coverage Diff @@
## master #3432 +/- ##
=======================================
Coverage 91.64% 91.64%
=======================================
Files 381 381
Lines 8268 8272 +4
Branches 227 225 -2
=======================================
+ Hits 7577 7581 +4
Misses 691 691
Continue to review full report at Codecov.
|
| * | ||
| * scala> val expectedResult = Option(SortedMap(false -> NonEmptyList.of(-2, -5), true -> NonEmptyList.of(12, 3))) | ||
| * | ||
| * scala> list.groupByNelA(num => Option(0).map(num >= _)) === expectedResult |
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
It works for you locally on 2.13?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I like this idea! I have two minor suggestions but think we should aim to get this into 2.2.0.
At first I wasn't 100% sure I liked the name, since there are lots of other places where we use F as a suffix even though we only have an Applicative constraint, but on second thought I think it's better to match e.g. foldMapA, filterA, etc. (and that inspectF, modifyF, etc. are the inconsistent ones).
|
|
||
| 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)) |
There was a problem hiding this comment.
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.
| * res0: Option[SortedMap[Boolean, NonEmptyList[Int]]] = Some(Map(false -> NonEmptyList(-2, -5), true -> NonEmptyList(12, 3))) | ||
| * }}} | ||
| */ | ||
| def groupByNelA[F[_], B](f: A => F[B])(implicit B: Order[B], F: Applicative[F]): F[SortedMap[B, NonEmptyList[A]]] = { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
|
seems like I adjusted the PR to all the suggestions, is there anything I should do more to get this merged? :) |
Closes #3420