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.

Sign up for GitHub

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

Jump to bottom

Add onError and adaptError to Applicative/MonadError #1739

Merged
merged 4 commits into from
Jul 24, 2017
Merged

Add onError and adaptError to Applicative/MonadError #1739

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ac43fa2
Allow ApplicativeError to execute callbacks on error and rethrow
SystemFw Jun 21, 2017
f6ac887
Allow MonadError to transform errors
SystemFw Jun 21, 2017
5c0cbcb
Simplify discipline tests for ApplicativeError
SystemFw Jul 20, 2017
c6223f3
Add doctests to `adaptError` and `onError`
SystemFw Jul 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions core/src/main/scala/cats/ApplicativeError.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ trait ApplicativeError[F[_], E] extends Applicative[F] {
def recoverWith[A](fa: F[A])(pf: PartialFunction[E, F[A]]): F[A] =
handleErrorWith(fa)(e =>
pf applyOrElse(e, raiseError))

/**
* Execute a callback on certain errors, then rethrow them.
*
* Any non matching error is rethrown as well.
*/
def onError[A](fa: F[A])(pf: PartialFunction[E, F[Unit]]): F[A] =
handleErrorWith(fa)(e =>
(pf andThen (map2(_, raiseError[A](e))((_, b) => b))) applyOrElse(e, raiseError))

/**
* Often E is Throwable. Here we try to call pure or catch
* and raise.
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/scala/cats/MonadError.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ trait MonadError[F[_], E] extends ApplicativeError[F, E] with Monad[F] {
def ensureOr[A](fa: F[A])(error: A => E)(predicate: A => Boolean): F[A] =
flatMap(fa)(a => if (predicate(a)) pure(a) else raiseError(error(a)))

/**
* Transform certain errors using `pf` and rethrow them.
*
* Non matching errors and successful values are not affected by this function
*/
def adaptError[A](fa: F[A])(pf: PartialFunction[E, E]): F[A] =
flatMap(attempt(fa))(_.fold(e => raiseError(pf.applyOrElse[E, E](e, _ => e)), pure))
}

object MonadError {
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/scala/cats/syntax/applicativeError.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,7 @@ final class ApplicativeErrorOps[F[_], E, A](val fa: F[A]) extends AnyVal {

def recoverWith(pf: PartialFunction[E, F[A]])(implicit F: ApplicativeError[F, E]): F[A] =
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is untested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
3 changes: 3 additions & 0 deletions core/src/main/scala/cats/syntax/monadError.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,7 @@ final class MonadErrorOps[F[_], E, A](val fa: F[A]) extends AnyVal {

def ensureOr(error: A => E)(predicate: A => Boolean)(implicit F: MonadError[F, E]): F[A] =
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is untested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

}
6 changes: 6 additions & 0 deletions laws/src/main/scala/cats/laws/ApplicativeErrorLaws.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ trait ApplicativeErrorLaws[F[_], E] extends ApplicativeLaws[F] {

def attemptFromEitherConsistentWithPure[A](eab: Either[E, A]): IsEq[F[Either[E, A]]] =
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)
Copy link
Contributor

@kailuowang kailuowang Jul 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor

@kailuowang kailuowang Jul 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@kailuowang kailuowang Jul 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

You are welcome. Thank you for contributing!


def onErrorRaise[A, B](fa: F[A], e: E, fb: F[B]): IsEq[F[A]] =
F.onError(F.raiseError[A](e)){case err => F.void(fb)} <-> F.map2(fb, F.raiseError[A](e))((_, b) => b)
}

object ApplicativeErrorLaws {
Expand Down
6 changes: 6 additions & 0 deletions laws/src/main/scala/cats/laws/MonadErrorLaws.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ trait MonadErrorLaws[F[_], E] extends ApplicativeErrorLaws[F, E] with MonadLaws[

def monadErrorEnsureOrConsistency[A](fa: F[A], e: A => E, p: A => Boolean): IsEq[F[A]] =
F.ensureOr(fa)(e)(p) <-> F.flatMap(fa)(a => if (p(a)) F.pure(a) else F.raiseError(e(a)))

def adaptErrorPure[A](a: A, f: E => E): IsEq[F[A]] =
F.adaptError(F.pure(a))(PartialFunction(f)) <-> F.pure(a)

def adaptErrorRaise[A](e: E, f: E => E): IsEq[F[A]] =
F.adaptError(F.raiseError[A](e))(PartialFunction(f)) <-> F.raiseError(f(e))
}

object MonadErrorLaws {
Expand Down
4 changes: 3 additions & 1 deletion laws/src/main/scala/cats/laws/discipline/ApplicativeErrorTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ trait ApplicativeErrorTests[F[_], E] extends ApplicativeTests[F] {
"applicativeError handleError consistent with recover" -> forAll(laws.handleErrorConsistentWithRecover[A] _),
"applicativeError recover consistent with recoverWith" -> forAll(laws.recoverConsistentWithRecoverWith[A] _),
"applicativeError attempt consistent with attemptT" -> forAll(laws.attemptConsistentWithAttemptT[A] _),
"applicativeError attempt fromEither consistent with pure" -> forAll(laws.attemptFromEitherConsistentWithPure[A] _)
"applicativeError attempt fromEither consistent with pure" -> forAll(laws.attemptFromEitherConsistentWithPure[A] _),
"applicativeError onError pure" -> forAll(laws.onErrorPure[A, B] _),
"applicativeError onError raise" -> forAll(laws.onErrorRaise[A, B] _)
)
}
}
Expand Down
4 changes: 3 additions & 1 deletion laws/src/main/scala/cats/laws/discipline/MonadErrorTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ trait MonadErrorTests[F[_], E] extends ApplicativeErrorTests[F, E] with MonadTes
def props: Seq[(String, Prop)] = Seq(
"monadError left zero" -> forAll(laws.monadErrorLeftZero[A, B] _),
"monadError ensure consistency" -> forAll(laws.monadErrorEnsureConsistency[A] _),
"monadError ensureOr consistency" -> forAll(laws.monadErrorEnsureOrConsistency[A] _)
"monadError ensureOr consistency" -> forAll(laws.monadErrorEnsureOrConsistency[A] _),
"monadError adaptError pure" -> forAll(laws.adaptErrorPure[A] _),
"monadError adaptError raise" -> forAll(laws.adaptErrorRaise[A] _)
)
}
}
Expand Down