"Move" type class syntax methods onto type classes#3152
Merged
travisbrown merged 16 commits intoNov 15, 2019
Conversation
Contributor
Author
|
Ǹote that there are no changes to tests and the only change to a doctest is to avoid the |
Codecov Report
@@ Coverage Diff @@
## master #3152 +/- ##
==========================================
- Coverage 93.19% 93.13% -0.07%
==========================================
Files 376 376
Lines 7323 7344 +21
Branches 195 180 -15
==========================================
+ Hits 6825 6840 +15
- Misses 498 504 +6
Continue to review full report at Codecov.
|
Member
|
I don't think I can review this in detail until tomorrow, but 👍 in principle. |
LukaJCB
approved these changes
Nov 14, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Between 1.0.0 and 2.0.0 we added a lot of new type class operations as syntax methods in order to avoid breaking binary compatibility on 2.11. We don't have to worry about that any more, which means these methods can go on the type classes where they belong.
Syntax methods
There are two categories of syntax methods that can be moved. The first is something like
findMforFoldable, which should live next tofindon the type class itself, but right now only exists as syntax forF[A]values:For methods like this I've added an implementation in the type class (
trait Foldablein this case):I've then changed the syntax implementation to use the new type class method. Note that the
@noopisn't strictly necessary (except in the case ofleftSequence, which I think hits a Simulacrum bug)—it's just an optimization to avoid Simulacrum generating redundant syntax methods (since we can't remove the current ones because of bincompat).The big advantage here is discoverability. Someone looking at the
FoldableAPI docs forfindmight wonder whether there's an effectful version, and there is, but they won't currently know that without also looking incats.syntax.foldable.Another advantage is ease of use. In some cases, like
countforUnorderedFoldable, which currently exists only as syntax, it's very difficult to actually use the method in many cases, because the syntax version collides with thecounton standard library collections. After this PR you can justF.count(pred)on yourUnorderedFoldableinstance.It's a smaller thing, but these changes also mean less logic in the syntax package, and fewer changes to make when we eventually do break binary compatibility in Cats 3.0 and (fingers crossed) can let Simulacrum or some Simulacrum successor do even more of the syntax boilerplate work.
Type class instance enrichment methods
The other category of methods I've moved here are things like
fromValidatedforApplicativeError. These methods aren't enriched ontoF[A]values or whatever, but onto the type class instance itself. In this case we can add the method to the type class and actually "remove" the syntax method, in the sense that we can make the enrichment conversion non-implicit and can deprecated everything involved (we can't remove remove it thanks to bincompat). This means less unnecessary garbage cluttering up implicit search when you import formcats.syntax.Other issues
We currently have
collectFirstandcollectFirstSomebutcollectFoldandcollectSomeFold(as syntax methods). I've usedcollectFoldSomefor consistency in the new proper type class version ofcollectFoldSome, and have deprecated thecollectFoldSomesyntax method.There are a few syntax methods that could have been added to type classes that I've omitted. Two examples are
contains_andmkString_forFoldable. In all of these cases (and there are only a couple others, I think) the implementations are trivial (like forcontains_) or the operation is very specific (likemkString_) and doesn't feel to me like it really needs to go on the type class.