2.13.0-RC1 #2792

travisbrown · 2019-04-14T11:45:09Z

Includes #2783 and #2791. Running the full build locally is apparently a mystery beyond my skill level so I'm just creating my own version of #2783 to run it here.

codecov-io · 2019-04-14T15:23:39Z

Codecov Report

Merging #2792 into master will decrease coverage by 0.12%.
The diff coverage is 38.88%.

@@            Coverage Diff             @@
##           master    #2792      +/-   ##
==========================================
- Coverage   94.28%   94.15%   -0.13%     
==========================================
  Files         367      368       +1     
  Lines        6926     6914      -12     
  Branches      304      296       -8     
==========================================
- Hits         6530     6510      -20     
- Misses        396      404       +8

Impacted Files	Coverage Δ
...in/scala/cats/kernel/instances/StaticMethods.scala	`73.8% <ø> (-19.05%)`	⬇️
...in/scala-2.12-/cats/kernel/compat/HashCompat.scala	`0% <0%> (ø)`
...main/scala/cats/kernel/instances/map/package.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/syntax/monadError.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/instances/sortedSet.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/ApplicativeError.scala	`100% <100%> (ø)`	⬆️
...n/scala/cats/kernel/instances/option/package.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/WriterT.scala	`91.93% <100%> (+0.06%)`	⬆️
.../src/main/scala/cats/syntax/applicativeError.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/MonadError.scala	`100% <100%> (ø)`	⬆️
... and 5 more

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 007b261...143a62b. Read the comment docs.

travisbrown · 2019-04-14T15:28:06Z

These two changes in 2.13.0-RC1 make a huge mess of some of the Hash instances in cats-kernel:

I really wish cats.kernel.instances.StaticMethods hadn't been made public—there's absolutely no good reason for it to be public, and the API needs to be different in a couple of places for 2.13. I've done that in a binary compatible way by just adding a few new methods (which I've made package private), but it's more of a mess than it needs to be.

In any case it seems to work now, and I've tried to minimize the version-specific code.

travisbrown · 2019-04-14T17:19:03Z

Ugh, there are some stray Hash instances for SortedSet and SortedMap in cats-core. Is there a reason these aren't in cats-kernel? The SortedSet instances even have catsKernel in the name. I've had to widen the access of product2HashWithPrefix and updateUnorderedHashC from private[kernel] to private[cats] because now we need them in core as well.

These aren't the only instances that seem out of place. At a glance:

travis@sidmouth cats(update/2.13.0-RC1)$ ag Kernel core/
core/src/main/scala/cats/data/Op.scala
23:  implicit def catsKernelEqForOp[Arr[_, _], A, B](implicit ArrEq: Eq[Arr[B, A]]): Eq[Op[Arr, A, B]] =

core/src/main/scala/cats/data/ZipList.scala
4:import cats.instances.list.catsKernelStdEqForList

core/src/main/scala/cats/instances/partialOrdering.scala
29:      def unit: PartialOrdering[Unit] = cats.instances.unit.catsKernelStdOrderForUnit.toOrdering

core/src/main/scala/cats/instances/sortedSet.scala
69:  implicit def catsKernelStdOrderForSortedSet[A: Order]: Order[SortedSet[A]] =
74:  implicit def catsKernelStdHashForSortedSet[A: Order: Hash]: Hash[SortedSet[A]] =
77:  implicit def catsKernelStdSemilatticeForSortedSet[A: Order]: BoundedSemilattice[SortedSet[A]] =

We're stuck with these in the wrong place until 3.0?

travisbrown · 2019-04-14T17:34:52Z

This is probably off-topic for this PR, but I have to say my experience contributing to Cats this weekend (for the first time in a year or so, at least with non-trivial changes) has been pretty miserable.

There are so many little broken things—these misnamed / misplaced instances, inconsistencies everywhere, #2790, stuff like StaticMethods that definitely should not be in the public API, the fact that the build is so complex (right at 1k lines for build.sbt and the Travis config) that I can't figure out how to run it locally in a useful way, the whole ScalaCheck / ScalaTest version mess, etc.—and I don't feel like I can do anything about any of it because any non-terrible-hack fixes break binary compatibility.

The worst part is that this is entirely self-inflicted: Cats 1.0 is almost a year and a half old—it would be perfectly reasonable to release a 99% source-compatible 2.0 that would break binary compatibility but also make the build hugely simpler and the code more consistent.

Okay, that's my rant—as I've been saying over and over for months I think the 2019 roadmap is a big mistake. Hopefully the build will pass this time and you can publish for 2.13.0-RC1 and I can go back to being just a reasonably happy library user instead of an extremely frustrated contributor.

travisbrown · 2019-04-14T18:14:10Z

Oh, cool, now I'm getting export-hook related errors locally in alleycats-tests, ~~but only on JDK 8—the tests run just fine on Java 10~~ (update: it's probably just flaky). Looking now…

travisbrown · 2019-04-14T19:30:20Z

Okay, the only failure is now the 2.13.0-RC1 JVM tests, and that's just an OOM. It might need attention but I just restarted to see if it was a one-off thing.

travisbrown · 2019-04-14T19:42:58Z

Now seeing the alleycats export-hook errors on 2.13.0-RC1 in CI:

[info] alleycats.tests.SetSuite *** ABORTED ***
[info]   java.lang.IncompatibleClassChangeError: Expected non-static field alleycats.std.ListInstances$exports$.alleycats$std$ListInstances$exports$$alleycats$std$ListInstances$anonimplicit$1
[info]   at alleycats.std.all$.<clinit>(all.scala:6)
[info]   at alleycats.tests.SetSuite.<init>(SetSuite.scala:12)
[info]   at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
[info]   at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
[info]   at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
[info]   at java.lang.reflect.Constructor.newInstance(Constructor.java:423)
[info]   at java.lang.Class.newInstance(Class.java:442)
[info]   at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:438)
[info]   at sbt.ForkMain$Run.lambda$runTest$1(ForkMain.java:304)
[info]   at java.util.concurrent.FutureTask.run(FutureTask.java:266)
[info]   ...

kailuowang · 2019-04-14T20:21:03Z

I plan to remove export hook. Will take on it tomorrow now that it is a blocker.

milessabin · 2019-04-15T08:42:39Z

+1 to removing the export-hook dependency, but even so ... why an IncompatibleClassChangeError? export-hook has been rebuilt for 2.13.0-RC1 so we really shouldn't be seeing that sort of thing.

travisbrown · 2019-04-15T08:48:50Z

@milessabin Yeah, it doesn't make sense to me. I've tried a full clean, etc., but it still happens maybe a quarter of the time both locally for me and in Travis CI.

kailuowang · 2019-04-16T16:19:19Z

@travisbrown, export hook is removed on master, if you merge master back in.
Note that currently, master branch is targeted to release 2.0, but we are waiting on Scala test 3.1.0 for that.

travisbrown · 2019-04-16T16:28:48Z

@kailuowang Great, thanks! Trying it out locally now.

kailuowang · 2019-04-17T01:17:19Z

core/src/main/scala/cats/data/WriterT.scala

@@ -357,6 +357,9 @@ sealed private[data] trait WriterTFlatMap1[F[_], L] extends WriterTApply[F, L] w
  implicit override def F0: FlatMap[F]
  implicit def L0: Monoid[L]

+  override def ap[A, B](f: WriterT[F, L, A => B])(fa: WriterT[F, L, A]): WriterT[F, L, B] =


Just curious, was this an optimization opportunity you happen to come across or was it broken on RC1?
Also would it be simpler to just call fa.ap(f)

This is from @SethTisue's commit, but the issue is that 2.13 sees the ap definitions in FlatMap and WriterTApply as conflicting. I personally think picking out the appropriate implementation via super is the right thing to do in this case, but I also don't think it matters much, and am happy to change it to fa.ap(f) if you'd prefer.

kailuowang · 2019-04-17T01:19:25Z

Thanks so much! Shall we remove the [WIP]? The changes look good to me. Just left a minor question.

travisbrown · 2019-04-17T07:36:28Z

@kailuowang I'd been thinking we'd merge #2791 separately and then I'd merge master back here, since the catalysts removal is logically separate—it just makes this update easier. We could also just merge this directly, though, so I've removed the WIP. Thanks for reviewing!

kailuowang

Thanks so much!

kailuowang · 2019-04-17T14:26:36Z

@travisbrown sorry I misunderstood your intention in regards to #2791. I agree that it's more logical to merge that as a separate PR. I am going to squash merge that one, and one you merge master back in this PR, please don't worry about sort of duplicated commits (the ones originally in #2791), we are going to squash merge this one too.

tpolecat · 2019-04-17T19:09:17Z

[error] /home/travis/build/typelevel/cats/build.sbt isn't formatted properly!

Well that's helpful.

travisbrown · 2019-04-17T20:10:24Z

@tpolecat Somehow in the merge build.sbt picked up a stray newline. But yeah, it's funny how irrationally annoying scalafmt's contentless error messages are, given that they're specifically not intended to be acted on (beyond indicating that you need to run scalafmt).

tpolecat

Anyway 👍 thanks for your work on this! With luck the scalafmt thing will be the last nit.

travisbrown and others added 7 commits April 13, 2019 09:30

Remove catalysts dependency

61718f3

Fix typo

b766494

Fix platform-specific stuff in alleycats

46ef642

Ugh, fix import

7c6c243

Format

2311c79

move to Scala 2.13.0-RC1 (drop -M5)

d1666de

Thanks to ScalaTest we temporarily need to use deprecated classes

757c13b

travisbrown force-pushed the update/2.13.0-RC1 branch from a82916d to 757c13b Compare April 14, 2019 13:31

Fix Hash instances to match changes in 2.13

7942148

travisbrown added 3 commits April 14, 2019 10:30

Fix for 2.11

7fdb2ce

One last fix for 2.13

d28da64

Fix Hash instances in cats-core for 2.13

abc53ce

Tell scalastyle to ignore returns in copy-pasted MurmurHash3 code

a27ecfe

travisbrown changed the title ~~2.13.0-RC1~~ 2.13.0-RC1 [WIP] Apr 14, 2019

kailuowang mentioned this pull request Apr 16, 2019

removed export hook #2794

Merged

Merge branch 'master' into update/2.13.0-RC1

5031281

SethTisue mentioned this pull request Apr 16, 2019

[WIP] move to Scala 2.13.0-RC1 (drop -M5) #2783

Closed

kailuowang reviewed Apr 17, 2019

View reviewed changes

kailuowang mentioned this pull request Apr 17, 2019

Remove catalysts dependency #2791

Merged

kailuowang added this to the 2.0 milestone Apr 17, 2019

travisbrown changed the title ~~2.13.0-RC1 [WIP]~~ 2.13.0-RC1 Apr 17, 2019

kailuowang previously approved these changes Apr 17, 2019

View reviewed changes

Merge branch 'master' into update/2.13.0-RC1

8cd5abb

kailuowang dismissed their stale review via 8cd5abb April 17, 2019 18:01

kailuowang previously approved these changes Apr 17, 2019

View reviewed changes

Format build.sbt

143a62b

travisbrown dismissed kailuowang’s stale review via 143a62b April 17, 2019 20:05

tpolecat approved these changes Apr 17, 2019

View reviewed changes

kailuowang merged commit 4b562a8 into typelevel:master Apr 17, 2019

kailuowang added the build label Apr 18, 2019

SethTisue mentioned this pull request Apr 24, 2019

publish for Scala 2.13.0-RC1, please #2776

Closed

joroKr21 mentioned this pull request Apr 27, 2019

Kittens 2.0 roadmap (resolve inconsistencies) typelevel/kittens#134

Closed

18 tasks

travisbrown mentioned this pull request Jul 10, 2019

Unifies NonEmpty datatypes on 2.13 to the Newtype encoding #2930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2.13.0-RC1 #2792

2.13.0-RC1 #2792

travisbrown commented Apr 14, 2019

codecov-io commented Apr 14, 2019 •

edited

Loading

travisbrown commented Apr 14, 2019

travisbrown commented Apr 14, 2019

travisbrown commented Apr 14, 2019

travisbrown commented Apr 14, 2019 •

edited

Loading

travisbrown commented Apr 14, 2019

travisbrown commented Apr 14, 2019

kailuowang commented Apr 14, 2019

milessabin commented Apr 15, 2019

travisbrown commented Apr 15, 2019

kailuowang commented Apr 16, 2019

travisbrown commented Apr 16, 2019

kailuowang Apr 17, 2019

travisbrown Apr 17, 2019

kailuowang commented Apr 17, 2019

travisbrown commented Apr 17, 2019

kailuowang left a comment

kailuowang commented Apr 17, 2019

tpolecat commented Apr 17, 2019

travisbrown commented Apr 17, 2019

tpolecat left a comment

2.13.0-RC1 #2792

2.13.0-RC1 #2792

Conversation

travisbrown commented Apr 14, 2019

codecov-io commented Apr 14, 2019 • edited Loading

Codecov Report

travisbrown commented Apr 14, 2019

travisbrown commented Apr 14, 2019

travisbrown commented Apr 14, 2019

travisbrown commented Apr 14, 2019 • edited Loading

travisbrown commented Apr 14, 2019

travisbrown commented Apr 14, 2019

kailuowang commented Apr 14, 2019

milessabin commented Apr 15, 2019

travisbrown commented Apr 15, 2019

kailuowang commented Apr 16, 2019

travisbrown commented Apr 16, 2019

kailuowang Apr 17, 2019

Choose a reason for hiding this comment

travisbrown Apr 17, 2019

Choose a reason for hiding this comment

kailuowang commented Apr 17, 2019

travisbrown commented Apr 17, 2019

kailuowang left a comment

Choose a reason for hiding this comment

kailuowang commented Apr 17, 2019

tpolecat commented Apr 17, 2019

travisbrown commented Apr 17, 2019

tpolecat left a comment

Choose a reason for hiding this comment

codecov-io commented Apr 14, 2019 •

edited

Loading

travisbrown commented Apr 14, 2019 •

edited

Loading