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

Make Parallel's F a type member #3012

Merged
merged 2 commits into from
Aug 26, 2019
Merged

Conversation

travisbrown
Copy link
Contributor

@travisbrown travisbrown commented Aug 26, 2019

Fixes #2233. We'd been holding off on this change because we thought it would necessarily require both source and binary compatibility breakage, but it turns out we can fix it without breaking binary compatibility with Cats 1.x.

Note that this change does break source compatibility pretty dramatically, but the migration path will generally be pretty clear (add .Aux, maybe an explicit type parameter here or there). Anyway, as Rob Norris says here, the current API is so awkward to use that this is unlikely to break much code.

With this change you can use parallel syntax without having to thread an extra type parameter everywhere through your code. Taking an example from the README of cats-par (a library for working around the limitations of the current API), this PR allows us to write the following:

import cats._
import cats.implicits._
import cats.data._

def withNewParallel[F[_]: Monad: Parallel, A, C, D](
  as: List[A],
  f: A => Kleisli[F, C, D]
): Kleisli[F, C, List[D]] =
  as.parTraverse(f)

With the current API we have to include an extra parameter G[_], both here and all the way up our call stack:

def withOldParallel[F[_]: Monad, G[_], A, C, D](
  as: List[A],
  f: A => Kleisli[F, C, D]
)(implicit P: Parallel[F, G]): Kleisli[F, C, List[D]] =
  as.parTraverse(f)

Note that the only changes to the tests are a few Auxs (and one fewer type parameter in one applicativeError call); otherwise the usage exercised in the tests didn't require changes.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! This has been a long time coming

@codecov-io
Copy link

codecov-io commented Aug 26, 2019

Codecov Report

Merging #3012 into master will decrease coverage by <.01%.
The diff coverage is 98.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3012      +/-   ##
=========================================
- Coverage    93.6%   93.6%   -0.01%     
=========================================
  Files         368     368              
  Lines        6948    6946       -2     
  Branches      188     185       -3     
=========================================
- Hits         6504    6502       -2     
  Misses        444     444
Impacted Files Coverage Δ
...ain/scala/cats/laws/discipline/ParallelTests.scala 100% <ø> (ø) ⬆️
...a/cats/laws/discipline/NonEmptyParallelTests.scala 100% <ø> (ø) ⬆️
core/src/main/scala/cats/syntax/parallel.scala 62.5% <0%> (ø) ⬆️
core/src/main/scala/cats/data/NonEmptyList.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/Parallel.scala 84.37% <100%> (ø) ⬆️
core/src/main/scala/cats/package.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/data/WriterT.scala 91.26% <100%> (-0.07%) ⬇️
...rc/main/scala/cats/laws/NonEmptyParallelLaws.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/data/Ior.scala 98.52% <100%> (ø) ⬆️
core/src/main/scala/cats/data/Kleisli.scala 90.9% <100%> (-0.09%) ⬇️
... and 5 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 14e525a...5d7ab29. Read the comment docs.

@mpilquist mpilquist self-requested a review August 26, 2019 16:39
@kailuowang
Copy link
Contributor

NIce, how come this (it doesn't break BC) didn't occur to us.

@kailuowang kailuowang merged commit 81bbbb2 into typelevel:master Aug 26, 2019
@kubukoz
Copy link
Member

kubukoz commented Aug 26, 2019

y'all reading in my mind, I thought about this just a couple days ago... a very welcome change!

@SethTisue
Copy link
Member

recent discussion at scala/scala#9303 has us wondering if this change was in fact safe. (which might affect decisions to make other such changes in the future)

@kubukoz
Copy link
Member

kubukoz commented Dec 7, 2020

Wow, took us long enough!

I can't remember any case in the ecosystem where this broke anything, but I would wager Parallel isn't that popular an abstraction in libraries, which could be why - everyone just updated cats and their sources and they were fine. (I think)

@SethTisue
Copy link
Member

Perhaps y'all should mimaReportSignatureProblems := true and see whether there are too many false positives.

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.

Use an abstract type member for the parallel type of the Parallel type class
8 participants