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.

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

Clean up some NonEmpty stuff #3307

Merged
merged 5 commits into from
Feb 20, 2020

Conversation

travisbrown
Copy link
Contributor

@travisbrown travisbrown commented Feb 20, 2020

Unfortunately cats.data is kind of a disaster, in part because all of the NonEmptyX types are unrelated and don't implement any particular interface. This means that not only do we have four different ways to encode non-empty collections, each of our implementations has a fairly arbitrary grab-bag of methods attached to it.

This PR adds several "missing" methods:

  • New for Chain: sortBy, sorted, zipWithIndex.
  • New for NonEmptyChain: sortBy, sorted, zipWithIndex, groupByNem, toNem, toNes, show.
  • New for NonEmptyLazyList: sortBy, sorted, groupBy, groupByNem, toNem, toNes, show.
  • New for NonEmptyList: iterator.
  • New for NonEmptyVector: iterator, groupBy, groupByNem, toNem, toNes.

This PR also introduces a new NonEmptyCollection interface that is implemented by NonEmptyChain, NonEmptyLazyList, NonEmptyList, and NonEmptyVector. It's roughly a union of the methods of these types, and the additions above were what was necessary to fill out these implementations. It's implemented as a universal trait, and does not interfere with value classes.

It's worth noting that NonEmptyCollection is not part of the public Cats API. The goal is to help us maintain consistency (and as a side effect to make testing easier), not to provide a user-facing abstraction. The user only sees a more consistent set of methods across these types.

I've also fixed one misnamed method on NonEmptyLazyList: collectLazyList (now deprecated) is clearly a typo for collectFirst.

It's worth noting that some methods that should be on NonEmptyCollection can't be, because of mistakes that we've made in the past. For example, NonEmptyChain has a groupBy that returns a NonEmptyMap, while the other NonEmptyX collections call that method groupByNem and have groupBy return a SortedSet. Also Chain has length and size methods that return a Long (?), while NonEmptyChain has a Long-returning length, but no size (??), while the other NonEmptyX collections have a length that returns an Int (and some of them have a size: Int). I can't wait for cats.data to be banished to its own module in Cats 3.

@kailuowang
Copy link
Contributor

Thanks! My sentiment towards these classes has always been slightly 👎 in having them in cats-core.

The more unified API introduced in this PR is nice and I agree that the long term plan should be moving them to their own module. Speaking of which, we might need to start thinking about which NonEmptyXXX to be used internally in cats-core.

@travisbrown
Copy link
Contributor Author

Thanks all! I'm going to merge this without squashing since the Chain additions are independent and even the other method additions are distinct from the NonEmptyCollection.

@travisbrown travisbrown merged commit 6eff820 into typelevel:master Feb 20, 2020
@travisbrown travisbrown deleted the topic/cats-data-fixes branch February 20, 2020 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants