Proof of concept creating a cats-kernel module for Cats/Algebra. #786

milessabin · 2016-01-07T13:37:18Z

This is a follow on from https://github.com/non/cats/pull/762 taking into consideration the discussion there and on typelevel/algebra#142.

This new PR has essentially the same content as #762 but instead of merging the Algebra type classes in with the core module it adds them to a new kernel module on which we can impose binary compatibility constraints that are sufficient to meet the Algebra project's needs.

ceedubs · 2016-01-07T13:46:51Z

core/src/main/scala/cats/std/bigInt.scala

@@ -1,7 +1,14 @@
 package cats
 package std

-trait BigIntInstances extends algebra.std.BigIntInstances {
+trait BigIntInstances {
+  implicit val bigIntAlgebra: BigIntAlgebra =


I think this is out of the scope of this PR, but just to jot it down while I'm thinking of it: is there any benefit to having this statically typed to BitIntAlgebra instead of just Order[BigInt]?

What are you proposing as an alternative?

I was just proposing : Order[BigInt] instead of : BigIntAlgebra for the return type. As far as I know that's the approach we are taking pretty much everywhere else.

Also, why not the Group[BigInt] here? We have it for Int, Long?

ceedubs · 2016-01-07T14:05:22Z

Is there a near-term plan for making laws/tests consistent between kernel and the rest of cats?

milessabin · 2016-01-07T14:26:19Z

@ceedubs this is a proof of concept for discussion. I thought it would be better to keep the edit distance from Algebra as small as possible for the time being. Once it's completely clear that this is the direction we and Algebra want to go then we should deal with laws/tests/docs/simulacrum as a matter of urgency.

ceedubs · 2016-01-07T14:30:04Z

@milessabin that makes sense, and I agree with your approach. We don't want this to turn into a never-ending PR.

This looks good to me. I'd like to see what some algebra committers such as @non, @johnynek, @TomasMikula and @rklaehn think.

ghost · 2016-01-07T15:02:07Z

we should deal with laws/tests/docs/simulacrum as a matter of urgency

I've been following all of this in Algebra/Cats and on many occasions I have desperately wanted to shout out about how catalysts can do this and that... but (thankfully) kept quiet so as not to interrupt the main discussion.

As you have all done a great job on consensus, now might be a good time to raise the point that sbt-catalysts/catalysts needs sorting in Cats/Algebra/Spire as well... on the build/testing side only.

johnynek · 2016-01-07T17:33:21Z

core/src/main/scala/cats/std/anyval.scala

-      def combine(x: Unit, y: Unit): Unit = ()
-      def empty: Unit = ()
-      def inverse(x: Unit): Unit = ()
+  implicit val unitOrder: Order[Unit] =


the group here can be useful no? (Allows you do use some Monads that want Monoids on some values, e.g. Writer?)

You mean plain Group rather than CommutativeGroup? Yes, agreed.

Yes, sorry. I overlooked that you just removed the CommutativeGroup, but that's another issue to keep in mind with standard instances: many standard ones are commutative, but kernel won't have it, so something for algebra to keep in mind.

johnynek · 2016-01-07T17:54:51Z

Re: typeclassic/simulacrum/machinist: can anyone confirm these have no runtime portion and are just generating code at compile time? Having a runtime portion makes the binary compat problem harder (obviously).

erik-stripe · 2016-01-07T18:20:15Z

This PR has the potential to get gigantic. Would folks be OK with creating a long-lived branch for this and then we can all issues PRs against it? That way we aren't relying on @milessabin to do all the work.

What do you all think?

johnynek · 2016-01-07T18:38:16Z

+1

On Thu, Jan 7, 2016 at 10:20 AM, Erik Osheim notifications@github.com
wrote:

This PR has the potential to get gigantic. Would folks be OK with creating
a long-lived branch for this and then we can all issues PRs against it?
That way we aren't relying on @milessabin https://github.com/milessabin
to do all the work.

—
Reply to this email directly or view it on GitHub
https://github.com/non/cats/pull/786#issuecomment-169762011.

P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

ceedubs · 2016-01-07T18:41:01Z

I agree that we shouldn't shove all of the work into this PR. However, if people are confident that this is the way forward, I might propose merging and making sure we have issues for the items we want to fix before a 1.0 release. That way we don't have a branch that potentially diverges significantly from master and becomes a constant struggle to keep up to date.

erik-stripe · 2016-01-07T20:18:47Z

The risk with merging now is that master will be in an unreleasable state until we straighten out the most pressing issues.

(For example, if cats-kernel does not export type class instances, then projects like algebra will have to either depend on cats-core or will have to duplicate those instances. IMO this is a flaw with the current PR.)

I think I'd prefer to do a branch which we will merge to master as soon as we feel like we hit a basic level of releasability. I'm also fine with merging now if other Cats devs aren't worried about this issue, but I just wanted to bring it up.

erik-stripe · 2016-01-07T20:21:46Z

The reason the cats-kernel issue is subtle is that cats.std is currently implemented using a package object, which can't exist in two sub-projects. Therefore, the ways to resolve this would be:

Put types from cats-kernel into cats.kernel (and cats.kernel.std) and then export/alias them into cats (and cats.std) in the cats-core sub-project.
Put the type class instances in a non-standard place (e.g. cats.kernel.std) and only realias those, leaving other types in cats.
Change how "std" is implemented so we don't need to use package objects, and can split these instances across cats-kernel and cats-core.
Something else I haven't thought of.

rklaehn · 2016-01-07T21:47:08Z

On Thu, Jan 7, 2016 at 9:18 PM, Erik Osheim notifications@github.com
wrote:

The risk with merging now is that master will be in an unreleasable
state until we straighten out the most pressing issues.

(For example, if cats-kernel does not export type class instances, then
projects like algebra will have to either depend on cats-core or will
have to duplicate those instances. IMO this is a flaw with the current PR.)

I would consider duplicate typeclass instances much less of a problem
than duplicate typeclasses. E.g. if you have Eq[Int] in both algebra.std
and cats.std, it might not be very nice from a DRY POV, but as long as both
are law-abiding, it is not the end of the world.

The only thing that would be a problem would be

import cats.implicits._
import algebra.implicits._

// Eq[Int] does not work because of duplicate implicits

But if there is a way to prevent this, I would be in favour of leaving
instances out of cats-kernel to keep cats-kernel small. I think there are
quite a large number of instances that you can provide for e.g. Eq and
Order. Eq for all primitive types, Strings, Arrays, scala collections,
Order for all kinds of sequences, PartialOrder for sets, ...

And the number of Monoid or Group instances for primitives, arrays, scala
collections, ... is also very large.

Do we really want them all in cats-kernel? If not, where do we draw the
line?

Another thing we should not forget is to rename empty to something else
(neutral?) before doing the first release that uses cats-kernel (but not
necessarily before this PR is merged to master)

I think I'd prefer to do a branch which we will merge to master as soon as
we feel like we hit a basic level of releasability. I'm also fine with
merging now if other Cats devs aren't worried about this.

—
Reply to this email directly or view it on GitHub
https://github.com/non/cats/pull/786#issuecomment-169794007.

erik-stripe · 2016-01-07T21:52:47Z

Yeah, I guess I am fine with not sharing instances, but we should be explicit that this is what is intended.

rklaehn · 2016-01-07T22:02:36Z

Not sharing instances would definitely make things easier for now. If we
decide to share instances there should at least be some clear criterion for
which instance we share and which we don't. E.g. you could make a case that
Eq[Int] belongs into cats-kernel, but I think the various ways to define
partial orders for sorted sets definitely do not belong in cats-kernel.

How would you deal with the problem of collisions when importing all
implicits of e.g. algebra and cats. Not at all / just tell people not to do
that?

On Thu, Jan 7, 2016 at 10:52 PM, Erik Osheim notifications@github.com
wrote:

Yeah, I guess I am fine with not sharing instances, but we should be
explicit that this is what is intended.

—
Reply to this email directly or view it on GitHub
https://github.com/non/cats/pull/786#issuecomment-169817557.

milessabin · 2016-01-08T09:22:58Z

I'm fine with this being a branch we can all contribute to ... shall I just do that?

milessabin · 2016-01-08T09:26:13Z

Unless I'm misunderstanding, I think that quite a few of @johnynek's comments would be equally applicable to Algebra as it currently stands and don't relate to issues that I've introduced in the process of moving things over?

If that's the case then I think that would be a very strong argument in favour of merging or making this into a branch that multiple parties can iterate on ... from my PoV this PR was just about the creation of cats-kernel, not also about refining its content.

non · 2016-01-08T13:15:54Z

@milessabin Yes, I think some of @johnynek's comments do apply to algebra currently. Let's try the branch idea (with an eye to merging to master relatively soon to avoid the situation @ceedubs mentions).

ceedubs · 2016-01-08T13:46:12Z

@Striation thanks, you cleared up to me why it isn't too great of an idea to merge to master right away. 👍 for an official repo branch for this.

milessabin · 2016-01-08T13:55:39Z

OK, I've rebased against master and pushed to https://github.com/non/cats/tree/topic/kernel.

rklaehn · 2016-01-13T16:17:19Z

Since there is now a separate branch to track this, I guess this PR should be closed and discussion continued somewhere else ( #777 )?

milessabin · 2016-01-14T09:28:03Z

Agreed ... we should capture the comments made here as separate issues.

I'm completely overloaded for the next couple of days, so any help with this would be appreciated.

julienrf · 2016-05-01T15:33:04Z

I think this issue can be closed now?

adelbertc · 2016-05-01T19:27:31Z

Fixed #1001

Proof of concept creating a cats-kernel module for Cats/Algebra.

4a76cf4

milessabin added the in progress label Jan 7, 2016

This was referenced Jan 7, 2016

Having non/algebra depends on non/cats typelevel/algebra#142

Closed

Proof of concept removing dependency on Algebra. #762

Closed

ceedubs reviewed Jan 7, 2016
View reviewed changes

Attempt to unbreak doc generation.

4aa9cfe

johnynek reviewed Jan 7, 2016
View reviewed changes

travisbrown mentioned this pull request Jan 7, 2016

Make Scaladoc (but not Unidoc) warnings non-fatal #787

Merged

travisbrown mentioned this pull request Jan 30, 2016

Let's talk the next release (0.4.0 edition) #834

Closed

5 tasks

ceedubs mentioned this pull request Feb 17, 2016

Optimize Eq[Vector[A]] instance #880

Merged

non mentioned this pull request Apr 25, 2016

Introduce cats-kernel and remove algebra dependency #1001

Merged

adelbertc closed this May 1, 2016

stew removed the in progress label May 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proof of concept creating a cats-kernel module for Cats/Algebra. #786

Proof of concept creating a cats-kernel module for Cats/Algebra. #786

milessabin commented Jan 7, 2016

ceedubs Jan 7, 2016

milessabin Jan 7, 2016

ceedubs Jan 7, 2016

johnynek Jan 7, 2016

ceedubs commented Jan 7, 2016

milessabin commented Jan 7, 2016

ceedubs commented Jan 7, 2016

ghost commented Jan 7, 2016

johnynek Jan 7, 2016

milessabin Jan 8, 2016

johnynek Jan 8, 2016

johnynek commented Jan 7, 2016

erik-stripe commented Jan 7, 2016

johnynek commented Jan 7, 2016

ceedubs commented Jan 7, 2016

erik-stripe commented Jan 7, 2016

erik-stripe commented Jan 7, 2016

rklaehn commented Jan 7, 2016

erik-stripe commented Jan 7, 2016

rklaehn commented Jan 7, 2016

milessabin commented Jan 8, 2016

milessabin commented Jan 8, 2016

non commented Jan 8, 2016

ceedubs commented Jan 8, 2016

milessabin commented Jan 8, 2016

rklaehn commented Jan 13, 2016

milessabin commented Jan 14, 2016

julienrf commented May 1, 2016

adelbertc commented May 1, 2016

Proof of concept creating a cats-kernel module for Cats/Algebra. #786

Proof of concept creating a cats-kernel module for Cats/Algebra. #786

Conversation

milessabin commented Jan 7, 2016

ceedubs Jan 7, 2016

Choose a reason for hiding this comment

milessabin Jan 7, 2016

Choose a reason for hiding this comment

ceedubs Jan 7, 2016

Choose a reason for hiding this comment

johnynek Jan 7, 2016

Choose a reason for hiding this comment

ceedubs commented Jan 7, 2016

milessabin commented Jan 7, 2016

ceedubs commented Jan 7, 2016

ghost commented Jan 7, 2016

johnynek Jan 7, 2016

Choose a reason for hiding this comment

milessabin Jan 8, 2016

Choose a reason for hiding this comment

johnynek Jan 8, 2016

Choose a reason for hiding this comment

johnynek commented Jan 7, 2016

erik-stripe commented Jan 7, 2016

johnynek commented Jan 7, 2016

ceedubs commented Jan 7, 2016

erik-stripe commented Jan 7, 2016

erik-stripe commented Jan 7, 2016

rklaehn commented Jan 7, 2016

erik-stripe commented Jan 7, 2016

rklaehn commented Jan 7, 2016

milessabin commented Jan 8, 2016

milessabin commented Jan 8, 2016

non commented Jan 8, 2016

ceedubs commented Jan 8, 2016

milessabin commented Jan 8, 2016

rklaehn commented Jan 13, 2016

milessabin commented Jan 14, 2016

julienrf commented May 1, 2016

adelbertc commented May 1, 2016