Add `onError` and `adaptError` to Applicative/MonadError by SystemFw · Pull Request #1739 · typelevel/cats

SystemFw · 2017-06-21T15:31:25Z

PR:
As per this Gitter conversation with @djspiewak, this PR adds two functions:

ApplicativeError#onError, to execute a callback on certain errors and rethrow:

onError[A](fa: F[A])(pf: PartialFunction[E, F[Unit]]): F[A]

with laws:

pure(a).onError(f) <-> pure(a)
raiseError(e).onError(err => fb) <-> fb *> raiseError(e)

Use case: error handling that respects module boundaries, e.g. rollback at the db layer, and then send a failed response at the http layer

MonadError#adaptError, to transform certain errors

def adaptError[A](fa: F[A])(pf: PartialFunction[E,E]): F[A]

with laws:

pure(a).adaptError(f) <-> pure(a)
raiseError(e).adaptError(f) <-> raiseError(f(e))

Use case: transforming errors (especially Throwable) between different layers of an application. The type is not as good as leftMap but the best we can have I think (except maybe constraining to Throwable).

A few things I'm not sure about code-wise:

Lack of syntax imports and trying to adhere to the existing style resulted in fairly "lispy" code
I couldn't get an Arbitrary[F[Unit]] in the laws, had to settle for F[B].void
Haven't added non-discipline tests (yet?), they all seem to be based on Option, for which none of these two functions happen to make sense
Should I add specialised implementations where relevant?

kailuowang · 2017-06-21T16:02:22Z

assigning @edmundnoble as well since he is removing ApplicativeError

codecov-io · 2017-06-21T17:29:03Z

Codecov Report

Merging #1739 into master will increase coverage by 1.27%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1739      +/-   ##
==========================================
+ Coverage      94%   95.27%   +1.27%     
==========================================
  Files         253      265      +12     
  Lines        4171     4298     +127     
  Branches      162       99      -63     
==========================================
+ Hits         3921     4095     +174     
+ Misses        250      203      -47

Impacted Files	Coverage Δ
core/src/main/scala/cats/MonadError.scala	`100% <100%> (ø)`	⬆️
.../src/main/scala/cats/syntax/applicativeError.scala	`100% <100%> (ø)`	⬆️
...a/cats/laws/discipline/ApplicativeErrorTests.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/syntax/monadError.scala	`100% <100%> (ø)`	⬆️
...rc/main/scala/cats/laws/ApplicativeErrorLaws.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/ApplicativeError.scala	`88.23% <100%> (+1.56%)`	⬆️
laws/src/main/scala/cats/laws/MonadErrorLaws.scala	`100% <100%> (ø)`	⬆️
...n/scala/cats/laws/discipline/MonadErrorTests.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/syntax/cartesian.scala	`50% <0%> (-25%)`	⬇️
core/src/main/scala/cats/instances/map.scala	`94.44% <0%> (-0.16%)`	⬇️
... and 52 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 4ba557c...c6223f3. Read the comment docs.

SystemFw · 2017-06-30T21:46:09Z

I'm waiting for #1751 to be resolved to update this PR (probably just moving stuff to MonadError)

johnynek · 2017-06-30T21:53:42Z

    F.recoverWith(fa)(pf)
+
+  def onError(fa: F[A])(pf: PartialFunction[E, F[Unit]])(implicit F: ApplicativeError[F, E]): F[A] =
+    F.onError(fa)(pf)


this is untested.

Yes, I mentioned this in the description :)

Haven't added non-discipline tests (yet?), they all seem to be based on Option, for which none of these two functions happen to make sense

Should I add a more complicated transformer to the tests to test this?

hmmm, since this is a simple delegation what about adding a simple doctest (maybe using Either) to the method in the type class?

Bear in mind that tests are missing for the implementation, not just for the syntax. I have added laws and discipline tests, but the reason I still haven't added unit tests is that all the unit tests I could find are based on Option, whereas testing this would require something like
EitherT[Writer, ...., I don't have a problem with it, just wanting to know if it's a good idea

I assumed that the discipline tests provides enough coverage for the implementation, no? Anyway I am not against adding a EitherT[Writer,... to the unit tests

Well, my idea was exactly that discipline tests might be enough, which is way I submitted this in its current form. Looking for advice from the maintainers to take the final decision

yeah, I think @johnynek and I were saying that ideally we also want to cover the delegation in the syntax, and one way to do that easily is to add a doctest in the type class method declaration, which uses the syntax and can also serve as an example in the scala doc.

I can't see any doctests for the other methods on ApplicativeError, apart from a scaladoc example for fromEither. Would you happen to have an example at hand of how syntax forwarders are tested in cats via doctests ?

Sure, here is one https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Apply.scala#L54

I'll add these today, especially since there's a bug 😁 (the fa should not be a parameter, shadows the implicit class parameter)

johnynek · 2017-06-30T21:53:47Z

    F.ensureOr(fa)(error)(predicate)
+
+  def adaptError(fa: F[A])(pf: PartialFunction[E, E])(implicit F: MonadError[F, E]): F[A] =
+    F.adaptError(fa)(pf)


this is untested.

Same as above

SystemFw · 2017-07-20T13:43:53Z

@edmundnoble @kailuowang @djspiewak Any news on this one? I'm still looking for advice on the testing, but apart from that, this would be nice to have for use with cats-effect IO, now that ApplicativeError seems to be staying in cats.

kailuowang · 2017-07-20T14:21:08Z

    F.attempt(F.fromEither(eab)) <-> F.pure(eab)
+
+  def onErrorPure[A, B](a: A, f: E => F[B]): IsEq[F[A]] =
+    F.onError(F.pure(a)){case err => F.void(f(err))} <-> F.pure(a)


Not quite sure I follow here, why not def onErrorPure[A, B](a: A, f: E => F[Unit])....? why take a E => F[B]

I've mentioned this in my comments, I couldn't get a Gen[F[Unit]]. However, it was my first time dealing with all the discipline infrastructure, and never had to to this particular thing in my own experience with scalacheck, so glad to be proven wrong :)

adding an ArbFU: Arbitrary[F[Unit]] param to the MonadErrorTests and ApplicativeErrorTest methods should do the trick

I think this is exactly what I did and iirc it didn't compile. Let me give it another try though :)

compiled for me, EitherTests passes. ~~running full now.~~ update: full suite passed.

Yes you're right. I hadn't realised I needed to add it to the MonadErrorTests as well (because of parent). Thank you! (I'll update the PR tonight with the change)

You are welcome. Thank you for contributing!

kailuowang · 2017-07-20T14:22:16Z

Also I think #1751 is close to merge. I think we can safely assume that ApplicativeError is going to stay.

SystemFw · 2017-07-21T17:07:07Z

@kailuowang I've simplified the discipline tests to take F[Unit], and added doctests for onError and applicativeError, let me know if there's any more feedback

kailuowang · 2017-07-21T18:10:03Z

👍 LGTM, thanks very much @SystemFw

edmundnoble · 2017-07-24T06:04:28Z

Merging with two approvals.

SystemFw added 2 commits June 21, 2017 16:27

Allow ApplicativeError to execute callbacks on error and rethrow

ac43fa2

Allow MonadError to transform errors

f6ac887

kailuowang assigned edmundnoble and djspiewak Jun 21, 2017

johnynek reviewed Jun 30, 2017

View reviewed changes

kailuowang reviewed Jul 20, 2017

View reviewed changes

SystemFw added 2 commits July 20, 2017 19:53

Simplify discipline tests for ApplicativeError

5c0cbcb

Add doctests to adaptError and onError

c6223f3

edmundnoble approved these changes Jul 22, 2017

View reviewed changes

edmundnoble merged commit 8571489 into typelevel:master Jul 24, 2017

kailuowang added this to the 1.0.0-MF milestone Jul 24, 2017

Uh oh!

Uh oh!

Conversation

SystemFw commented Jun 21, 2017

Uh oh!

kailuowang commented Jun 21, 2017

Uh oh!

codecov-io commented Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

SystemFw commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SystemFw commented Jul 20, 2017

Uh oh!

kailuowang Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang commented Jul 20, 2017

Uh oh!

SystemFw commented Jul 21, 2017

Uh oh!

kailuowang commented Jul 21, 2017

Uh oh!

edmundnoble commented Jul 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Jun 21, 2017 •

edited

Loading

SystemFw commented Jun 30, 2017 •

edited

Loading

kailuowang Jul 20, 2017 •

edited

Loading

kailuowang Jul 20, 2017 •

edited

Loading

kailuowang Jul 20, 2017 •

edited

Loading