Skip to content

Add missing methods to NE collections and syntax#3998

Draft
satorg wants to merge 13 commits into
typelevel:mainfrom
satorg:prepend-colls-to-nevs
Draft

Add missing methods to NE collections and syntax#3998
satorg wants to merge 13 commits into
typelevel:mainfrom
satorg:prepend-colls-to-nevs

Conversation

@satorg

@satorg satorg commented Sep 23, 2021

Copy link
Copy Markdown
Contributor

The initial discussion is here.

The following methods are added:

  • NonEmptyList:
    • prependList
  • ListOps:
    • concatNel
  • VectorOps:
    • concatNev
  • SeqOps:
    • concatNeSeq
    • groupByNeSeq (similar to ListOps#groupByNel and VectorOps#groupByNev)
    • groupByNeSeqA (similar to ListOps#groupByNelA and VectorOps#groupByNevA)
    • scanLeftNeSeq (similar to ListOps#scanLeftNel and VectorOps#scanLeftNev)
    • scanRightNeSeq (similar to ListOps#scanRightNel and VectorOps#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.

@satorg satorg changed the title Add missing methods to NE data stuctures Add missing methods to NE data structures Sep 23, 2021
@satorg satorg changed the title Add missing methods to NE data structures Add missing methods to NE collections and syntax Sep 23, 2021
@satorg
satorg force-pushed the prepend-colls-to-nevs branch from b0ea384 to ec4ca2e Compare September 24, 2021 05:11
@satorg
satorg force-pushed the prepend-colls-to-nevs branch from ec4ca2e to e70f22a Compare September 24, 2021 06:54
@satorg
satorg force-pushed the prepend-colls-to-nevs branch from 1706004 to c70efc8 Compare September 25, 2021 21:43
@satorg
satorg marked this pull request as ready for review September 26, 2021 01:45

@johnynek johnynek left a comment

Copy link
Copy Markdown
Contributor

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](

Copy link
Copy Markdown
Contributor

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))

Copy link
Copy Markdown
Contributor Author

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.

@satorg satorg Oct 1, 2021

Copy link
Copy Markdown
Contributor Author

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.

@satorg satorg Oct 1, 2021

Copy link
Copy Markdown
Contributor Author

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:

  1. It is too verbose and may require explicit types.
  2. 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.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

@johnynek

johnynek commented Oct 1, 2021

Copy link
Copy Markdown
Contributor

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).

@satorg
satorg marked this pull request as draft November 4, 2021 03:24
@armanbilge armanbilge added this to the 2.9.0 milestone Jun 11, 2022
@armanbilge armanbilge modified the milestones: 2.9.0, 2.10.0 Oct 9, 2022
@armanbilge armanbilge modified the milestones: 2.10.0, 2.11.0 Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants