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

Unifies NonEmpty datatypes on 2.13 to the Newtype encoding #2930

Closed
wants to merge 26 commits into from

Conversation

kailuowang
Copy link
Contributor

@kailuowang kailuowang commented Jul 3, 2019

As of now there are four types of NonEmptyXXX data types in Cats.

  1. NonEmptyList a dedicated wrapper for a List tail and head.
  2. NonEmptyStream developed using OneAnd abstraction
  3. NonEmptyVector a value class wrapper for Vector
  4. NonEmptySet, NonEmptyMap, NonEmptyChain, new type implementation of Set, Map, and Chain.

This PR unifies them all to the newtype encoding 4 but only on Scala 2.13, on Scala 2.12- things are mostly kept to maintain BC.

This PR also added a new NonEmptyLazyList data type on 2.13, fixes #2903

@LukaJCB LukaJCB self-requested a review July 3, 2019 19:39
@codecov-io
Copy link

codecov-io commented Jul 3, 2019

Codecov Report

Merging #2930 into master will decrease coverage by 0.68%.
The diff coverage is 90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2930      +/-   ##
==========================================
- Coverage   94.03%   93.34%   -0.69%     
==========================================
  Files         370      369       -1     
  Lines        7038     6796     -242     
  Branches      186      185       -1     
==========================================
- Hits         6618     6344     -274     
- Misses        420      452      +32
Impacted Files Coverage Δ
.../scala/cats/kernel/instances/StreamInstances.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/data/package.scala 90.9% <ø> (ø) ⬆️
...rc/main/scala/cats/laws/discipline/arbitrary.scala 99.15% <ø> (ø)
core/src/main/scala/cats/data/NonEmptySet.scala 97.61% <ø> (ø) ⬆️
...la/cats/data/AbstractNonEmptyBimonadTraverse.scala 100% <100%> (ø)
core/src/main/scala/cats/data/NonEmptyChain.scala 64.48% <78.26%> (-32.84%) ⬇️
core/src/main/scala/cats/Reducible.scala 100% <0%> (+4.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e053e28...fee01b8. Read the comment docs.

LukaJCB
LukaJCB previously approved these changes Jul 4, 2019
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for this huge effort @kailuowang!

LukaJCB
LukaJCB previously approved these changes Jul 8, 2019
Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm strongly 👎 on having significant differences between e.g. NonEmptyList implementations on 2.13 and pre-2.13 Scala versions. Even if we could be 100% confident that this would never cause problems for people cross-building non-version-specific code (which I'm at most maybe 30% confident about), I'm not sure it's worth the confusion for people reading the codebase or trying to reason about performance across Scala versions.

If there's a good reason to make breaking changes (and in my view there is and has been for months), let's bundle them up and make a breaking change, not smuggle them in via version-specific shenanigans like this.

@kailuowang
Copy link
Contributor Author

Thanks for your feedback @travisbrown. I am going to take this chance to rethink our 2.0-3.0 strategy.
In the meantime I just want to add some quick response to some of your ponts.

Even if we could be 100% confident that this would never cause problems for people cross-building non-version-specific code (which I'm at most maybe 30% confident about)

I am 95% confident, since all the existing tests are still cross building without the need for version specific stuff, but for rare cases it shouldn't be hard to add version specific code, they probably already have the machinery to support 2.13

trying to reason about performance across Scala versions

Not sure if that will be a big issue, different Scala versions have different performance implications.

... make a breaking change, not smuggle them in via version-specific shenanigans

I disagree with your suggestion that this PR is smuggling breaking changes. For a change to be breaking it has to be breaking from past binaries. In this case, there are no past (officially released) binary to be broken from. Users who upgrades from 1.6 to 2.0 while migrating to or adding support for Scala 2.13 won't experience any breaking changes.

@travisbrown
Copy link
Contributor

@kailuowang If the argument is "we want to standardize these as soon as possible" and "as soon as possible" is now for 2.13, then why couldn't we make the same argument for other things that have been blocked by bincompat? Why not have a separate source tree for 2.13 with no BinCompat traits, etc.? Moving 2.13 forward in just this one respect because we don't have bincompat constraints seems inconsistent and confusing.

@kailuowang
Copy link
Contributor Author

why couldn't we make the same argument for other things that have been blocked by bincompat?

That's exactly my rethinking is about. I am drafting a new plan for 2.0 now. Will submit the issue soon.

@travisbrown
Copy link
Contributor

@kailuowang To address your comment on Gitter:

I understand your concern over parallel source tree. It would be a big concern for most libraries, but I think Cats is unique in two ways: 1, feature wise, Cats is quite stable now. 2, Cats sits at the very bottom of the dependency graph and is used widely. So I think we should consider starting plan Cats like how we would plan for Scala stdlib.

My more general concern here is that the Cats maintainership (and I'm including myself) has a pretty poor track record when it comes to managing complexity and promoting consistency. Things are constantly slipping into the public API that definitely should not be there, there are instances all over the place that don't follow naming / location conventions, etc. I don't trust us to maintain separate source trees for 2.13 and pre-2.13 versions without making mistakes that will lead to pain for other library maintainers and a worse experience for users.

@travisbrown
Copy link
Contributor

(That last comment may come across as more negative than I intend—I think Cats is a great library, I use it every day, I'm proud of the small part I've played in helping to build it, and I appreciate the work everyone else is doing. I just think we need to be realistic about the limited resources we have in terms of maintainer attention and effort, and correspondingly thoughtful about what we prioritize.)

@kailuowang
Copy link
Contributor Author

I don't have much else to say at this point, but just want to clarify on so called "a pretty poor track record" which is referring to the following statement, among other complaints in the linked issue.

inconsistencies everywhere, #2790, stuff like StaticMethods that definitely should not be in the public API

Both the inconsistency in #2790 and StaticMethods were introduced when we fold algebra-core into cats-kernel 3 years ago. IIRC, we chose to retain as much as backward compatibility with algebra at the time so that algebra can smoothly migrate to depend on cats-kernel instead.

@travisbrown
Copy link
Contributor

Okay, if that's the direction people want to go I'd be happy to be proven wrong!

@kailuowang
Copy link
Contributor Author

I extracted the less contentious NonEmptyLazyList to #2941

@kailuowang
Copy link
Contributor Author

replaced by @2941 based on feedback.

@kailuowang kailuowang closed this Aug 22, 2019
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.

On Scala 2.13 replace NonEmptyStream with NonEmptyLazyList
5 participants