-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add MonadError instance for EitherT that recovers from F[_] errors. #1644
Changes from 12 commits
43eab48
979d869
4148d21
ca76937
aa6a955
ab37c2b
a09fe4c
6d41f14
3882f81
5eae9e4
c554233
f477698
eee4315
cd04677
3160827
d10318b
b7e1760
e7b5590
eabb6f2
bdc44a8
b5425bd
abce798
839f1be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package cats | ||
package data | ||
|
||
import cats.arrow.FunctionK | ||
import cats.functor.Bifunctor | ||
import cats.instances.either._ | ||
import cats.syntax.either._ | ||
|
@@ -91,6 +92,14 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) { | |
|
||
def map[D](f: B => D)(implicit F: Functor[F]): EitherT[F, A, D] = bimap(identity, f) | ||
|
||
def transformF[G[_]](fe: FunctionK[F, G]): EitherT[G, A, B] = EitherT(fe(value)) | ||
|
||
def recoverF[E](pf: PartialFunction[E, B])(implicit me: MonadError[F, E]): EitherT[F, A, B] = | ||
EitherT(me.recover(value)(pf.andThen(b => Right(b)))) | ||
|
||
def recoverFWith[E](pf: PartialFunction[E, F[Either[A, B]]])(implicit me: MonadError[F, E]): EitherT[F, A, B] = | ||
EitherT(me.recoverWith(value)(pf)) | ||
|
||
def semiflatMap[D](f: B => F[D])(implicit F: Monad[F]): EitherT[F, A, D] = | ||
flatMap(b => EitherT.right(f(b))) | ||
|
||
|
@@ -483,9 +492,13 @@ private[data] abstract class EitherTInstances1 extends EitherTInstances2 { | |
new EitherTBitraverse[F] { | ||
val F0: Traverse[F] = F | ||
} | ||
|
||
implicit def catsDataMonadErrorFForEitherT[F[_], E, L](implicit FE0: MonadError[F, E]): MonadError[EitherT[F, L, ?], E] = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick: would you add a comment doc here to indicate that this instance delegate to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kailuowang sure, I'm on it. |
||
new EitherTMonadErrorF[F, E, L] { implicit val F = FE0 } | ||
} | ||
|
||
private[data] abstract class EitherTInstances2 extends EitherTInstances3 { | ||
|
||
implicit def catsDataMonadErrorForEitherT[F[_], L](implicit F0: Monad[F]): MonadError[EitherT[F, L, ?], L] = | ||
new EitherTMonadError[F, L] { | ||
implicit val F = F0 | ||
|
@@ -547,6 +560,15 @@ private[data] trait EitherTMonad[F[_], L] extends Monad[EitherT[F, L, ?]] with E | |
})) | ||
} | ||
|
||
private[data] trait EitherTMonadErrorF[F[_], E, L] extends MonadError[EitherT[F, L, ?], E] with EitherTMonad[F, L] { | ||
implicit val F: MonadError[F, E] | ||
|
||
def handleErrorWith[A](fea: EitherT[F, L, A])(f: E => EitherT[F, L, A]): EitherT[F, L, A] = | ||
EitherT(F.handleErrorWith(fea.value)(f(_).value)) | ||
|
||
def raiseError[A](e: E): EitherT[F, L, A] = EitherT.left(F.raiseError(e)) | ||
} | ||
|
||
private[data] trait EitherTMonadError[F[_], L] extends MonadError[EitherT[F, L, ?], L] with EitherTMonad[F, L] { | ||
def handleErrorWith[A](fea: EitherT[F, L, A])(f: L => EitherT[F, L, A]): EitherT[F, L, A] = | ||
EitherT(F.flatMap(fea.value) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
package cats | ||
package tests | ||
|
||
import cats.arrow.FunctionK | ||
import cats.data.EitherT | ||
import cats.functor.Bifunctor | ||
import cats.functor._ | ||
import cats.laws.discipline._ | ||
import cats.laws.discipline.arbitrary._ | ||
import cats.kernel.laws.{OrderLaws, GroupLaws} | ||
import cats.kernel.laws.{GroupLaws, OrderLaws} | ||
|
||
|
||
class EitherTTests extends CatsSuite { | ||
implicit val iso = CartesianTests.Isomorphisms.invariant[EitherT[ListWrapper, String, ?]](EitherT.catsDataFunctorForEitherT(ListWrapper.functor)) | ||
|
@@ -46,17 +48,24 @@ class EitherTTests extends CatsSuite { | |
|
||
{ | ||
//if a Monad is defined | ||
import cats.instances.option | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why this line is needed. I think |
||
|
||
implicit val F = ListWrapper.monad | ||
implicit val eq0 = EitherT.catsDataEqForEitherT[ListWrapper, String, Either[String, Int]] | ||
implicit val eq1 = EitherT.catsDataEqForEitherT[EitherT[ListWrapper, String, ?], String, Int](eq0) | ||
implicit val eq2 = EitherT.catsDataEqForEitherT[Option, String, Either[String, String]] | ||
implicit val eq3 = EitherT.catsDataEqForEitherT[EitherT[Option, String, ?], String, String](eq2) | ||
implicit val me = EitherT.catsDataMonadErrorFForEitherT[Option, Unit, String](option.catsStdInstancesForOption) | ||
|
||
Functor[EitherT[ListWrapper, String, ?]] | ||
Applicative[EitherT[ListWrapper, String, ?]] | ||
Monad[EitherT[ListWrapper, String, ?]] | ||
MonadTrans[EitherT[?[_], String, ?]] | ||
|
||
checkAll("EitherT[ListWrapper, String, Int]", MonadErrorTests[EitherT[ListWrapper, String, ?], String].monadError[Int, Int, Int]) | ||
checkAll("EitherT[Option, String, String]", MonadErrorTests[EitherT[Option, String, ?], Unit].monadError[String, String, String]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. another nitpick, would you add a comment indicating that this is testing the |
||
checkAll("MonadError[EitherT[List, ?, ?]]", SerializableTests.serializable(MonadError[EitherT[ListWrapper, String, ?], String])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we need to law tests the new instance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kailuowang I really did that by copying and it seems to give the coverage to the new MonadError instance. If it is not this way, can you please give me a hint? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kailuowang I still think that it is useful to have the recoverF/With functions but if it is a blocker for the PR to not be approved then I could remove them and keep the new MonadError instance and the transformF function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I apologize, @leandrob13, I meant serializable test the instance. Not sure what I was thinking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kailuowang no problem, got it! |
||
checkAll("MonadError[EitherT[Option, ?, ?]]", SerializableTests.serializable(MonadError[EitherT[Option, String, ?], Unit])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should separate these {
// If a MonadError is defined for F
val eq = ...
checkAll("EitherT[Option, String, String]", ...)
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @peterneyens no problem, I can do that. |
||
checkAll("MonadTrans[EitherT[?[_], String, ?]]", MonadTransTests[EitherT[?[_], String, ?]].monadTrans[ListWrapper, Int, Int]) | ||
} | ||
|
||
|
@@ -219,9 +228,33 @@ class EitherTTests extends CatsSuite { | |
eithert.recoverWith { case "noteithert" => EitherT.pure[Id, String](5) } should === (eithert) | ||
} | ||
|
||
test("recoverWith ignores the right side") { | ||
val eithert = EitherT.pure[Id, String](10) | ||
eithert.recoverWith { case "eithert" => EitherT.pure[Id, String](5) } should === (eithert) | ||
test("recoverF recovers handled values") { | ||
val et: EitherT[Option, String, Int] = EitherT.right[String](Option.empty[Int]) | ||
et.recoverF[Unit] { case u => 5 }.value.get should === (Right(5)) | ||
} | ||
|
||
test("recoverF ignores the non error in F") { | ||
val et: EitherT[Option, String, Int] = EitherT.right[String](Some(1)) | ||
et.recoverFWith[Unit] { case u => Some(Right(5)) }.value.get should === (Right(1)) | ||
} | ||
|
||
test("recoverFWith recovers handled values") { | ||
val et: EitherT[Option, String, Int] = EitherT.right[String](Option.empty[Int]) | ||
et.recoverFWith[Unit] { case u => Some(Right(5)) }.value.get should === (Right(5)) | ||
} | ||
|
||
test("recoverFWith ignores the non error in F") { | ||
val et: EitherT[Option, String, Int] = EitherT.right[String](Some(1)) | ||
et.recoverFWith[Unit] { case u => Some(Right(5)) }.value.get should === (Right(1)) | ||
} | ||
|
||
test("transformF consistent with transforming value") { | ||
val toOptionK: FunctionK[List, Option] = new FunctionK[List, Option] { | ||
def apply[A](fa: List[A]): Option[A] = fa.headOption | ||
} | ||
forAll { (eithert: EitherT[List, String, Int]) => | ||
eithert.transformF(toOptionK).value should === (toOptionK(eithert.value)) | ||
} | ||
} | ||
|
||
test("transform consistent with value.map") { | ||
|
@@ -339,6 +372,17 @@ class EitherTTests extends CatsSuite { | |
} | ||
} | ||
|
||
test("collectRight with Id consistent with Either foldRight") { | ||
val et: EitherT[Option, String, Int] = EitherT.pure(1) | ||
et.collectRight should === (Some(1)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouls this use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @LukaJCB I am changing the name and the test. I forgot to fix this! |
||
} | ||
|
||
test("applyAlt with Id consistent with EitherT map") { | ||
forAll { (et: EitherT[Id, String, Int], f: Int => String) => | ||
et.applyAlt(EitherT.pure(f)) should === (et.map(f)) | ||
} | ||
} | ||
|
||
test("merge with Id consistent with Either merge") { | ||
forAll { (x: EitherT[Id, Int, Int]) => | ||
x.merge should === (x.value.merge) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In accordance with the corresponding method on
Kleisli
orReaderT
being called onlytransform
, maybe we should name it this way also? See #1724Edit: I see now, that
EitherT
already has a function calledtransform
, so we might need to find a different solution