-
-
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
"Move" type class syntax methods onto type classes #3152
"Move" type class syntax methods onto type classes #3152
Conversation
Ǹ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.
|
I don't think I can review this in detail until tomorrow, but 👍 in principle. |
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.
👍
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
findM
forFoldable
, which should live next tofind
on 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 Foldable
in this case):I've then changed the syntax implementation to use the new type class method. Note that the
@noop
isn'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
Foldable
API docs forfind
might 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
count
forUnorderedFoldable
, which currently exists only as syntax, it's very difficult to actually use the method in many cases, because the syntax version collides with thecount
on standard library collections. After this PR you can justF.count(pred)
on yourUnorderedFoldable
instance.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
fromValidated
forApplicativeError
. 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
collectFirst
andcollectFirstSome
butcollectFold
andcollectSomeFold
(as syntax methods). I've usedcollectFoldSome
for consistency in the new proper type class version ofcollectFoldSome
, and have deprecated thecollectFoldSome
syntax 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.