More consistency #3000
More consistency #3000
Conversation
| @@ -9,7 +9,7 @@ import annotation.tailrec | |||
|
|
|||
| trait FunctionInstances extends cats.kernel.instances.FunctionInstances with Function0Instances with Function1Instances | |||
|
|
|||
| trait FunctionInstancesBinCompat0 { | |||
| private[instances] trait FunctionInstancesBinCompat0 { | |||
There was a problem hiding this comment.
I am not sure about introducing a BinCompat trait to fix a typo. The plan is to branch out scala 2.11 code right after 2.0 release. Which means we shall be able to leave the typo in the 2.11 code and fix it in master branch without an extra bincompat trait.
There was a problem hiding this comment.
I can pull these parts out, but in my view it'd be better to get all the instance name-related source compat breakage out of the way at once, instead of spreading it across 2.0 and 2.1.
There was a problem hiding this comment.
The source breakage is highly unlikely to affect anyone (and if anyone it would be a super quick fix on compilation time), but the extra trait will be left there for a very long time.
There was a problem hiding this comment.
So you're saying that breaking source compat in 2.x releases is acceptable?
|
|
||
| override def eqv(s1: SortedSet[A], s2: SortedSet[A]): Boolean = | ||
| StaticMethods.iteratorEq(s1.iterator, s2.iterator) | ||
| private[instances] trait SortedSetInstancesBinCompat1 extends LowPrioritySortedSetInstancesBinCompat1 { |
There was a problem hiding this comment.
same concern here, introducing a bincomp trait to fix an inconsistent placement while we should be able to fix this right after 2.0 release without it
| @@ -80,7 +80,7 @@ therefore name our implicits according to the following rules: | |||
| - If the instance is for multiple type classes, use `InstancesFor` instead of a type class name. | |||
| - If the instance is for a standard library type add `Std` after the package. i.e. `catsStdShowForVector` and `catsKernelStdGroupForTuple`. | |||
|
|
|||
| As an example, an implicit instance of `Monoid` for `List` defined in the package `Kernel` should be named `catsKernelMonoidForList`. | |||
| As an example, an implicit instance of `Monoid` for `List` defined in the package `Kernel` should be named `catsKernelStdMonoidForList`. | |||
There was a problem hiding this comment.
I am not sure if the renaming of these catskernelXXXX instances are worth the extra code due to BC
The std in implicit names in cats-core was due to the fact that cats.instances package was used to be cats.std. And instances in cats.kernel.instances don't have the instance package name in their names because having instances in an instance name is kind of redundant. We have this long name rule in place simply so that contributors can easily come up with long unique name for these implicit defs to avoid potential conflict with other libraries. I agree that ideally we should keep it consistent but I think we are paying too much a cost for this kind of rename fix.
Codecov Report
@@ Coverage Diff @@
## master #3000 +/- ##
==========================================
- Coverage 93.75% 93.13% -0.62%
==========================================
Files 366 368 +2
Lines 6935 6981 +46
Branches 185 184 -1
==========================================
Hits 6502 6502
- Misses 433 479 +46
Continue to review full report at Codecov.
|
|
Closing this since the important parts got split out and merged in separate PRs. Maybe we can revisit the rest after the 2.11 code split. |
Update
Because some of these fixes are more controversial than I expected, I've split the easy ones out into separate PRs:
I'm hoping we can get at least these merged before 2.0, even if the rest has to wait for 2.1.
Original description
There's currently a lot of inconsistency in Cats, both in the implementation and in the public API. I know this isn't something most people care about, but I hate it, and even for normal people I imagine it has to make contributing and reading the code harder than it should be. This PR aims to fix a few things that can be fixed without breaking binary compatibility in Cats 2, and to put us in a better position to make things even less horrible in Cats 3!
It does break source compatibility, it breaks it all over the place, but in ways that shouldn't affect normal usage. This is part of the reason I'd really like to get these changes into 2.0.0.
Misnamed instances
Most of these changes are pretty mechanical. I see an
Eqinstance namedzipNelEq, I get extremely angry, I deprecate it, remove itsimplicit, make it package private, copy the implementation, rename itcatsDataEqForZipNonEmptyListor whatever it is it actually should be, make surevalidateBCstill passes on all Scala versions, and then repeat dozens of times for the rest of the misnamed stuff. In some cases I've also had to add newBinCompattraits for misnamed instances defined in traits (thanks to 2.11).SortedSet and SortedMap instances
This is one of the bigger changes. Previously we only had instances for cats-kernel type classes (
Eq,Monoid, etc.) forSortedSetandSortedMapin cats-core. I don't know why, but I don't think there's a good reason for it. To make things even more confusing, some of the instances for these types in cats-core followed the cats-kernel instance naming convention. I've moved these instances into cats-kernel and done all the necessary juggling to keep from breaking bincompat in cats-core.Cleaner public API
I've also made all (or approximately all)
BinCompatand prioritization traits private to the smallest possible enclosing package (I'm pretty confident theBinCompatones are all done, but the prioritization ones are easier to miss). Some of these were already package-private, but apparently at random. There's no good reason for them to be in the public interface, so they're gone now.