-
-
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
Add missing methods to NE collections and syntax #3998
base: main
Are you sure you want to change the base?
Conversation
b0ea384
to
ec4ca2e
Compare
ec4ca2e
to
e70f22a
Compare
1706004
to
c70efc8
Compare
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.
To me this highlights a weakness of the NonEmpty
types...
I can imagine:
trait Reducible[F[_]] {
type Empty[_]
def emptyFoldable: Foldable[Empty]
def toEmpty[A](f: F[A]): Empty[A]
def fromEmpty[A](f: Empty[A]): Option[F[A]]
}
trait NonEmptyTraverse[F[_]] extends Reducible[F] {
type Empty[_]
def emptyTraverse: Traverse[Empty]
}
We could potentially add a typeclass:
trait HasNonEmpty[F[_], TC[_[_]]] {
type NonEmpty[_]
def emptyTC: TC[F]
def nonEmptyTC: TC[NonEmpty]
def toEmpty[A](f: F[A]): Empty[A]
def fromEmpty[A](f: Empty[A]): Option[F[A]]
}
and then use the HasNonEmpty[List, Traverse]
, HasNonEmpty[Vector, Traverse]
, etc...
* res0: Boolean = true | ||
* }}} | ||
*/ | ||
def groupByNevA[F[_], B]( |
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.
I wonder if this should live here:
https://github.com/typelevel/cats/blob/main/core/src/main/scala/cats/NonEmptyTraverse.scala
def groupByA[G[_], B](fn: A => G[B])(implicit G: Apply[G], B: Order[B]): G[SortedMap[B, F[A]]]
and then you can call that here:
toNev.fold(F.pure(SortedMap.empty[B, NonEmptyVector[A]])(_.groupByA(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.
Well, these particular methods are just an attempt to maintain some degree of consistency and interoperability among all linear "non-empty" collections we have in cats.data
. Because initially there was ListOps#groupByNelA
only but no corresponding methods in VectorOps
nor SeqOps
. Which is a bit confusing.
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.
But I like the idea of generalization. I also have a feeling, that keeping stuffing various *Ops
classes with almost identical methods is not the best way to deal with it. I like your suggestion in particular, but why just NonEmptyTraverse
?
Seems, it should work with just a regular Traverse
– shouldn't it?
I mean, when we do grouping on some collection by some predicate, all the groups we will get eventually are going to be non-empty regardless of whether the source collection was empty or not. I.e. we will either get a non-empty group or don't get it at all.
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.
We can use the way you suggested of course:
toNev.fold(F.pure(SortedMap.empty[B, NonEmptyVector[A]])(_.groupByA(f))
But to me it has at least two flaws:
- It is too verbose and may require explicit types.
- It kind of "disguises" the idea of type safety for grouping: i.e. the grouping over a regular collection ALWAYS results in zero or more groups of corresponding non-empty collections.
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.
to your point 2: putting groupByA
on NonEmptyTraverse
we could actually return NonEmptyMap[B, F[A]]
, so we know both the map is non-empty and since F[_]: NonEmptyTraverse
we also know F[_]
is non-empty.
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.
Yes, this is a valid case too. But I'd suppose that grouping over a regular Traverse
is a more common case anyway. And it's guaranteed to produce a regular Map
of NonEmpty*
groups. While grouping over NonEmptyTraverse
is guaranteed to produce a NonEmptyMap
of NonEmpty*
groups. I mean both cases looks legit to me and I'd like to think on how to incorporate both of them.
I appreciate the time you have taken to do this. I find it hard to review code that just adds ad-hoc methods to enrich standard types. It's difficult to know that we are being consistent or that we aren't missing a more general concept that should properly be added to a typeclass (which is the main focus of cats). |
The initial discussion is here.
The following methods are added:
NonEmptyList
:prependList
ListOps
:concatNel
VectorOps
:concatNev
SeqOps
:concatNeSeq
groupByNeSeq
(similar toListOps#groupByNel
andVectorOps#groupByNev
)groupByNeSeqA
(similar toListOps#groupByNelA
andVectorOps#groupByNevA
)scanLeftNeSeq
(similar toListOps#scanLeftNel
andVectorOps#scanLeftNev
)scanRightNeSeq
(similar toListOps#scanRightNel
andVectorOps#scanRightNev
)All the new methods are covered by appropriate tests.
Note: this PR is based on #3931. Since by this moment the former PR has not been merged yet, these new methods show up here too:
VectorOps
:groupByNev
groupByNevA
scanLeftNev
scanRightNev
Technically this PR supersedes the previous one therefore if all changes from both PRs are accepted, then we can just go ahead and merge this PR only and close the other.