I1061 part 1 - renames in std package #1066
Conversation
| @@ -14,37 +14,37 @@ trait AnyValInstances | |||
| with TupleInstances | |||
|
|
|||
| trait IntInstances extends cats.kernel.std.IntInstances { | |||
| implicit val intShow: Show[Int] = Show.fromToString[Int] | |||
| implicit val catsShowForcatsForInt: Show[Int] = Show.fromToString[Int] | |||
There was a problem hiding this comment.
You might have gone a little overboard here ;)
| @@ -23,7 +23,7 @@ trait EitherInstances extends EitherInstances1 { | |||
| } | |||
| } | |||
|
|
|||
| implicit def eitherInstances[A]: Monad[Either[A, ?]] with Traverse[Either[A, ?]] = | |||
| implicit def catsMonadForEither[A]: Monad[Either[A, ?]] with Traverse[Either[A, ?]] = | |||
There was a problem hiding this comment.
For this and several other instances you've somewhat arbitrarily picked the first first type class when an instanced provides several type classes. This may not be a big deal and I don't think I really have a better suggestion, but it does feel a little off.
There was a problem hiding this comment.
I see, I used a regex replace, so it inherited the original name which didn't have the Traverse in it. I meant to have all typeclass in it though as I did with several manual replacement. updated
There was a problem hiding this comment.
There's probably no single good solution here, but I think that including all of the type class names in the implicit name has its issues too. Changing the name of these implicits breaks binary compatibility. Ideally we could make an implicit extend another type class without breaking binary compatibility but then we wouldn't have the new type class in the name.
There was a problem hiding this comment.
Yeah, I thought about using a name like catsInstancesForXXX but it might conflict with cats.kernel
There was a problem hiding this comment.
As per #1061 (comment), perhaps the full package name should be incuded? So here, cats => catsStd. An then catsKernel, catsKernelStd and so on
There was a problem hiding this comment.
@ceedubs I don't recall any situation where this sort of thing came up. Having said that, I now wonder whether these instances are necessarily orphan, or whether we can come up with a non-orphan encoding which doesn't duplicate code all over the place. (If they were not orphan this problem wouldn't have existed in the first place.)
|
I am fixing the build. Also if no objection I am going to implement @inthenow 's suggestion ( full package name). for instances with multiple typeclasses I am just going to name them with |
Current coverage is 88.37%@@ master #1066 diff @@
==========================================
Files 215 215
Lines 2743 2743
Methods 2689 2689
Messages 0 0
Branches 49 49
==========================================
Hits 2424 2424
Misses 319 319
Partials 0 0
|
|
This is great, thanks! Can you also update the names in |
|
@non I updated the kernelBoilerplate and |
|
@kailuowang I was wrong. It looks like you got them all! Thanks! 👍 |
|
👍 |
No description provided.