Skip to content
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

Merged
merged 16 commits into from
Nov 15, 2019

Conversation

travisbrown
Copy link
Contributor

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 for Foldable, which should live next to find on the type class itself, but right now only exists as syntax for F[A] values:

final class FoldableOps[F[_], A](private val fa: F[A]) extends AnyVal {
  def findM[G[_]](p: A => G[Boolean])(implicit F: Foldable[F], G: Monad[G]): G[Option[A]] = ???
}

For methods like this I've added an implementation in the type class (trait Foldable in this case):

  @noop
  def findM[G[_], A](fa: F[A])(p: A => G[Boolean])(implicit G: Monad[G]): G[Option[A]] =

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 of leftSequence, 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 for find might wonder whether there's an effectful version, and there is, but they won't currently know that without also looking in cats.syntax.foldable.

Another advantage is ease of use. In some cases, like count for UnorderedFoldable, which currently exists only as syntax, it's very difficult to actually use the method in many cases, because the syntax version collides with the count on standard library collections. After this PR you can just F.count(pred) on your UnorderedFoldable 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 for ApplicativeError. These methods aren't enriched onto F[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 form cats.syntax.

Other issues

  • We currently have collectFirst and collectFirstSome but collectFold and collectSomeFold (as syntax methods). I've used collectFoldSome for consistency in the new proper type class version of collectFoldSome, and have deprecated the collectFoldSome syntax method.

  • There are a few syntax methods that could have been added to type classes that I've omitted. Two examples are contains_ and mkString_ for Foldable. In all of these cases (and there are only a couple others, I think) the implementations are trivial (like for contains_) or the operation is very specific (like mkString_) and doesn't feel to me like it really needs to go on the type class.

@travisbrown travisbrown added this to the 2.1.0-RC1 milestone Nov 14, 2019
@travisbrown
Copy link
Contributor Author

Ǹote that there are no changes to tests and the only change to a doctest is to avoid the collectSomeFold deprecation. This is a breaking change for people who for some strange reason are using e.g. ApplicativeErrorExtensionOps explicitly, but shouldn't affect any normal usage.

@codecov-io
Copy link

codecov-io commented Nov 14, 2019

Codecov Report

Merging #3152 into master will decrease coverage by 0.06%.
The diff coverage is 92.06%.

Impacted file tree graph

@@            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
Flag Coverage Δ
#scala_version_212 93.47% <92.06%> (-0.04%) ⬇️
#scala_version_213 90.71% <65.07%> (-0.14%) ⬇️
Impacted Files Coverage Δ
.../src/main/scala/cats/syntax/applicativeError.scala 81.25% <0%> (-18.75%) ⬇️
core/src/main/scala/cats/Reducible.scala 100% <100%> (ø) ⬆️
...re/src/main/scala/cats/syntax/traverseFilter.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/Foldable.scala 98.48% <100%> (+0.31%) ⬆️
core/src/main/scala/cats/FlatMap.scala 91.66% <100%> (+5%) ⬆️
core/src/main/scala/cats/syntax/validated.scala 100% <100%> (ø) ⬆️
...src/main/scala/cats/syntax/unorderedFoldable.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/syntax/flatMap.scala 75% <100%> (-5.96%) ⬇️
core/src/main/scala/cats/syntax/apply.scala 50% <100%> (-7.15%) ⬇️
core/src/main/scala/cats/syntax/bitraverse.scala 100% <100%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 038d683...7b30d5b. Read the comment docs.

@rossabaker
Copy link
Member

I don't think I can review this in detail until tomorrow, but 👍 in principle.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@travisbrown travisbrown merged commit 291af38 into typelevel:master Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants