Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

@travisbrown travisbrown requested a review from fthomas November 14, 2019 09:59
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
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MonadError#adaptError could be on ApplicativeError instead
4 participants