Add distinctBy to NonEmptyCollection and all impls #4608
Conversation
| override def distinct[AA >: A](implicit O: Order[AA]): NonEmptyLazyList[AA] = distinctBy(identity[AA]) | ||
|
|
||
| override def distinctBy[AA >: A, B](f: A => B)(implicit O: Order[B]): NonEmptyLazyList[AA] = { |
There was a problem hiding this comment.
I'm concerned quite a bit about this type boundary: AA >: A.
It doesn't seem doing any useful work here.
Although the former method distinct requires that type boundary to overcome a variance error, e.g.:
covariant type A occurs in invariant position in type cats.Order[A] of value O
or something like that.
However, in distinctBy it doesn't seem necessary and can only make calling distinctBy more difficult in some specific cases.
There was a problem hiding this comment.
The same concern applies to the other distinctBy methods down below.
There was a problem hiding this comment.
Although the former method distinct requires that type boundary to overcome a variance error,
Just for a record, I feel the widening of the result type in the original distinct implementation does not seem necessary either. I believe, it could be just this:
def distinct[AA >: A](implicit O: Order[AA]): NonEmptyLazyList[A] // <- can return `A`, not `AA`There was a problem hiding this comment.
Also just for a record, looking at the original implementation I find it far from efficient: it uses foldLeft which is not really necessary there. Especially when results get accumulated in a mutable collection (ListBuffer in that case). I think it would be more efficient to use a regular loop there to avoid creation of anonymous functions and closures.
UPDATE: sorry, forgot that it is a lazy list. A regular loop wouldn't work well for this particular collection. But for the other non-lazy ones it would do. Anyway, it is just for a record, no need to address it in this PR – it would be out of its scope.
There was a problem hiding this comment.
I'm concerned quite a bit about this type boundary:
AA >: A. It doesn't seem doing any useful work here.Although the former method
distinctrequires that type boundary to overcome a variance error, e.g.:covariant type A occurs in invariant position in type cats.Order[A] of value O
or something like that.
However, in
distinctByit doesn't seem necessary and can only make callingdistinctBymore difficult in some specific cases.
Thanks for your suggestion. I've simplified the type signature for distinctBy, as per your suggestion.
distinct is unchanged.
tests/shared/src/test/scala-2.13+/cats/tests/NonEmptyLazyListSuite.scala
Outdated
Show resolved
Hide resolved
|
Thank you for the contribution! Despite my nitpicks above I really appreciate your work and believe that |
cybersaurus
force-pushed
the
NonEmptyCollection-distinctBy
branch
from
5e78f33
to
a39d75f
Compare
Thanks for your feedback 👍 I think I've addressed all your comments now. |
|
The errors in the native build are on suites that I otherwise can't find in the logs. I don't think they're related to this, but I don't really pay attention to Native, so I'm not the best one to help: |
I don't think either. I noticed that after migration to native 0.5 the build became a bit flaky. I'll try to re-run the jobs. |
|
The build has succeeded after the re-run 🎉 I wonder though what is wrong with Scala Native 0.5 that makes our build flaky 🤔 |
First time contributor... 👋
What
distinctBytoNonEmptyCollectionand implementations.Why
ListtoNonEmptyListI noticed thatdistinctBywas missing. This addition adds parity withdistinct/distinctByin the scala standard library, and also withcats.data.Chain.