Introduce cats-kernel and remove algebra dependency #1001
Conversation
This commit sets up the basic SBT subproject configuration and moves the core algebra type classes into the cats.kernel package.
Algebra doesn't currently provide generic semigroups for number types, so this commit doesn't have any of those either. Cats is already working around this so that shouldn't be a big change.
There are a few things here: 1. Many of algebra's interesting laws apply to rings/lattices. 2. We introduce a (laws-only) dependency on catalysts-platform. 3. Unlike Cats, we'll run the tests in kernel-laws' tests.
This commit ports over the law tests which make sense in the context of cats-kernel. For some reason the tests aren't being run right now, but at least they compile.
After this commit, the intention is that none of the algebra types are used. The tests pass and things seem to work. Next step is to actually remove the algebra dependency.
After clean, validate passed. I think this branch is good to go.
|
I'm also noticing that some files and large blocks are entirely commented out. Feel free to point these out -- I'll be removing them as I find them. |
|
|
||
| implicit val intShow: Show[Int] = | ||
| Show.fromToString[Int] | ||
|
|
||
| implicit val intGroup: CommutativeGroup[Int] = | ||
| AdditiveCommutativeGroup[Int].additive | ||
|
|
||
| new CommutativeGroup[Int] { |
There was a problem hiding this comment.
should this not be in cats.kernel.std.IntInstances?
There was a problem hiding this comment.
I was going to ask you about that -- we didn't have generic semigroups for numbers in Algebra. If everyone agrees that we want those I can move them in this PR.
| @@ -194,16 +195,16 @@ object Order extends OrderFunctions { | |||
| * | |||
| * @see [[Order.whenEqual]] | |||
| */ | |||
| def whenEqualMonoid[A]: Monoid[Order[A]] = | |||
| new Monoid[Order[A]] { | |||
| def whenEqualMonoid[A]: Band[Order[A]] = | |||
There was a problem hiding this comment.
I think we want Band[Order[A]] with Monoid[Order[A]] Band does not have empty but BoundedSemilattice is commutative.
|
It seems like codecov.io may have a partial outage. I'm not sure the commits will turn green, but Travis passed. EDIT: Seems like I may have lowered coverage -- I'll have to work on increasing it before we merge I guess. |
| lazy val kernelSettings = Seq( | ||
| scalacOptions ++= commonScalacOptions.filter(_ != "-Ywarn-value-discard"), | ||
| resolvers ++= Seq( | ||
| "bintray/non" at "http://dl.bintray.com/non/maven", |
There was a problem hiding this comment.
That may not be necessary -- I'm not sure what we're using it for. I'll try removing it.
Current coverage is 74.16%@@ master #1001 diff @@
==========================================
Files 368 434 +66
Lines 2367 2945 +578
Methods 2162 2668 +506
Messages 0 0
Branches 16 59 +43
==========================================
+ Hits 1983 2184 +201
- Misses 384 761 +377
Partials 0 0
|
|
👍 |
| implicit def listMonoid[A] = new ListMonoid[A] | ||
| } | ||
|
|
||
| trait ListInstances1 extends ListInstances2 { |
There was a problem hiding this comment.
I'm inclined to make traits like ListInstances1 and ListInstances2 sealed and private[cats.kernel.std] just to signify that they are an internal detail and not really meant to be used directly. I think we are taking this approach in most of cats.
There was a problem hiding this comment.
It's always easier to remove sealed and private than it is to add them in later.
|
Thanks a bunch for all of your work on this @non! |
This does the following: 1. Fixes a lot of return types. 2. Removes cats.kernel.std.array 3. Removes the unused cats.kernel.Priority 4. Improves some comments 5. Makes a small efficiency improvement for Monoid[Map[K, V]]
This fixes the merge conflicts and also re-removes algebra.
|
@ceedubs I think I took every piece of advice you gave except about the mix-ins, which I want to wait to hear from Oscar about. |
Current coverage is 74.83%@@ master #1001 diff @@
==========================================
Files 368 430 +62
Lines 2365 2912 +547
Methods 2157 2639 +482
Messages 0 0
Branches 20 57 +37
==========================================
+ Hits 1981 2179 +198
- Misses 384 733 +349
Partials 0 0
|
Current coverage is 74.83%@@ master #1001 diff @@
==========================================
Files 368 430 +62
Lines 2365 2912 +547
Methods 2157 2639 +482
Messages 0 0
Branches 20 57 +37
==========================================
+ Hits 1981 2179 +198
- Misses 384 733 +349
Partials 0 0
|
|
Seems like the coverage is actually pretty low. Check: Looks like we just need to connect a lot of the instances to the laws, but also some of the core methods are not tested, for instance: pmin, pmax, reverse look untested. |
| */ | ||
| def tryCompare(x: A, y: A): Option[Int] = { | ||
| val c = partialCompare(x, y) | ||
| if (c.isNaN) None else Some(c.signum) |
There was a problem hiding this comment.
this boxes doesn't it? Don't we need java.lang.Double.isNaN(c)?
There was a problem hiding this comment.
Good catch! Yes, I'll change it.
There was a problem hiding this comment.
also, we might keep val some1 = Some(1); val someNeg1 = Some(-1); val someZero = Some(0) around and not allocate on each compare. These could be private in the PartialOrder object.
|
@johnynek Regarding coverage: Yes, we're missing a lot of things. One thing I want to do is write laws to ensure that |
|
@non is it fair to assume that this PR has as much coverage as was previously present in algebra for the same code? If so, I would probably advocate for getting this PR merged (after the 0.5 cats release) and then following up with code coverage PRs for two reasons: 1) so you aren't forced to do all of the work and 2) to avoid the hassle of repeated merge conflicts. |
|
+1 on merging and beefing up coverage after. |
This commit also adds some "missing" function traits (as abstract classes) and replaces an instance of c.isNaN with isNaN(c) for performance.
|
OK I think all the extant issues (except for coverage) have been addressed. |
|
👍 thanks again for all of your work on this, @non! Are we waiting until after the cats 0.5 release for this? |
|
👍 awesome work! Thanks for picking up where I left off :-) |
|
@ceedubs Yes, I think we should do one more release before merging this. |
Current coverage is 81.99%
@@ master #1001 diff @@
==========================================
Files 368 215 -153
Lines 2376 2704 +328
Methods 2169 2639 +470
Messages 0 0
Branches 19 60 +41
==========================================
+ Hits 1994 2217 +223
- Misses 382 487 +105
Partials 0 0
|
|
Looks like master merged cleanly. I'm fine with merging this anytime. |
Cats Kernel
Overview
This branch pulls the code from algebra-core and algebra-laws into cats-kernel and cats-kernel-laws (respectively). The intention here is to reverse the Algebra/Cats dependency using the intersection of code that they are both interested in.
We already have two PRs that address this same issue (#786 and #978). Thanks to @milessabin and @has609 for that work -- I'm sorry I wasn't able to better communicate what I thought the requirements for this PR were (and that I couldn't get this PR done faster). I am proposing we merge this PR instead of those.
In order to satisfy everyone in Cats and Algebra as much as possible, I tried not to editorialize and just do the minimum job necessary get this working. However, I think we have the following requirements:
Dependencies
Algebra needs to be able to depend on cats-kernel and have everything that currently works continue to work. This means that we need to export the same instances that Algebra uses here (rather than migrating the instances to cats-core). This also means that we need to use the package
cats.kernelrather thancats, since otherwise we have a collision around who provides thecats.stdpackage object.This also means that we need to export
cats-kernel-lawsseparately fromcats-laws, and stick to the the current layout/strategy Algebra uses (for now).No new dependencies
Cats-kernel should not introduce new dependencies that Algebra didn't already have. I was able to accommodate this except for an extra compile-time dependency from cats-kernel-laws onto catalysts-platform (to get serialization tests working correctly on JVM and disabled on JS), which I thought was worth it and didn't pose a big risk.
Style issues
Initially we need to start from the Algebra code, even if it is in a different style than Cats currently. We should be open to PRs improving the kernel, but the expectation is that this code needs to be very stable, and should also be less opinionated (to support Algebra's goal that it be usable from Algebird, Breeze, Spire, etc.). This means that we aren't using Simulacrum or Machinist for now, not supporting operators/syntax in the kernel, and being very explicit.
In some cases (i.e. the cats-kernel / algebra law testing code) I think we might want to try to adopt Cats' style, but let's leave that conversation until after this PR is merged if possible.
Administrative issues
Even though we are moving this code to the Cats repo, I'd like to imagine that we'll solicit the Algebra maintainers' opinions before making big changes to this code. Would we be willing to consider the Algebra maintainers as Cats maintainers for the purposes of working in the Kernel? I'd also like to freeze this code sooner than later, and begin doing MiMA compatibility testing to ensure robust binary compatibility ASAP.
Conclusion
I'd like to get sign-offs from at least two other Algebra maintainers on this PR in addition to the usual sign-offs from Cats maintainers. I'm happy to make substantive edits to this PR if we can get consensus amongst those two groups that the changes are warranted.
Also, I'd like to merge #994 and get another Cats release out before we actually merge this PR -- let's discuss the timing of the first Algebra-free release here as well.