-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Proof of concept creating a cats-kernel module for Cats/Algebra. #786
Conversation
@@ -1,7 +1,14 @@ | |||
package cats | |||
package std | |||
|
|||
trait BigIntInstances extends algebra.std.BigIntInstances { | |||
trait BigIntInstances { | |||
implicit val bigIntAlgebra: BigIntAlgebra = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you proposing as an alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why not the Group[BigInt]
here? We have it for Int, Long?
Is there a near-term plan for making laws/tests consistent between kernel and the rest of cats? |
@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. |
@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. |
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 |
def combine(x: Unit, y: Unit): Unit = () | ||
def empty: Unit = () | ||
def inverse(x: Unit): Unit = () | ||
implicit val unitOrder: Order[Unit] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the group here can be useful no? (Allows you do use some Monads that want Monoids on some values, e.g. Writer?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean plain Group
rather than CommutativeGroup
? Yes, agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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). |
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? |
+1 On Thu, Jan 7, 2016 at 10:20 AM, Erik Osheim [email protected]
P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin |
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. |
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. |
The reason the cats-kernel issue is subtle is that
|
On Thu, Jan 7, 2016 at 9:18 PM, Erik Osheim [email protected]
I would consider duplicate typeclass instances much less of a problem The only thing that would be a problem would be import cats.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 And the number of Monoid or Group instances for primitives, arrays, scala Do we really want them all in cats-kernel? If not, where do we draw the Another thing we should not forget is to rename empty to something else
|
Yeah, I guess I am fine with not sharing instances, but we should be explicit that this is what is intended. |
Not sharing instances would definitely make things easier for now. If we How would you deal with the problem of collisions when importing all On Thu, Jan 7, 2016 at 10:52 PM, Erik Osheim [email protected]
|
I'm fine with this being a branch we can all contribute to ... shall I just do that? |
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. |
@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). |
@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. |
OK, I've rebased against master and pushed to https://github.com/non/cats/tree/topic/kernel. |
Since there is now a separate branch to track this, I guess this PR should be closed and discussion continued somewhere else ( #777 )? |
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. |
I think this issue can be closed now? |
Fixed #1001 |
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.