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 adaptError #3148

Merged
merged 7 commits into from
Nov 14, 2019
Merged

Conversation

travisbrown
Copy link
Contributor

Fixes #2685.

Unfortunately I don't think we can fix the syntax without breaking bincompat due to a compiler bug: #2685 (comment)

@codecov-io
Copy link

codecov-io commented Nov 13, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@bd82ff2). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3148   +/-   ##
=========================================
  Coverage          ?   93.28%           
=========================================
  Files             ?      376           
  Lines             ?     7323           
  Branches          ?      200           
=========================================
  Hits              ?     6831           
  Misses            ?      492           
  Partials          ?        0
Flag Coverage Δ
#scala_version_212 93.62% <100%> (?)
#scala_version_213 90.94% <100%> (?)
Impacted Files Coverage Δ
.../src/main/scala/cats/syntax/applicativeError.scala 100% <ø> (ø)
laws/src/main/scala/cats/laws/MonadErrorLaws.scala 100% <ø> (ø)
...n/scala/cats/laws/discipline/MonadErrorTests.scala 100% <ø> (ø)
core/src/main/scala/cats/MonadError.scala 100% <ø> (ø)
...a/cats/laws/discipline/ApplicativeErrorTests.scala 100% <100%> (ø)
core/src/main/scala/cats/ApplicativeError.scala 100% <100%> (ø)
...rc/main/scala/cats/laws/ApplicativeErrorLaws.scala 100% <100%> (ø)

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 bd82ff2...a727fbf. Read the comment docs.

fthomas
fthomas previously approved these changes Nov 14, 2019
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.

There are conflicts now, but looks good in principle.

@@ -124,8 +124,7 @@ final class ApplicativeErrorOps[F[_], E, A](private val fa: F[A]) extends AnyVal
* }}}
*
* This is the same as `MonadErrorOps#adaptError`. It cannot have the same name because
* this would result in ambiguous implicits. `adaptError` will be moved from `MonadError`
* to `ApplicativeError` in Cats 2.0: see [[https://github.com/typelevel/cats/issues/2685]]
* this would result in ambiguous implicits.
*/
def adaptErr(pf: PartialFunction[E, E])(implicit F: ApplicativeError[F, E]): F[A] =
F.recoverWith(fa)(pf.andThen(F.raiseError[A] _))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: is there any reason to keep a duplicate implementation, now that it's on the constraint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, thanks for catching this!

@travisbrown
Copy link
Contributor Author

Okay, conflict is resolved and implementation is de-duplicated. I've also added some syntax tests.

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 1d1b261 into typelevel:master Nov 14, 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.

MonadError#adaptError could be on ApplicativeError instead
4 participants