-
-
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 22 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 |
---|---|---|
|
@@ -6,7 +6,8 @@ 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,6 +47,7 @@ class EitherTTests extends CatsSuite { | |
|
||
{ | ||
//if a Monad is defined | ||
|
||
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) | ||
|
@@ -54,13 +56,30 @@ class EitherTTests extends CatsSuite { | |
Applicative[EitherT[ListWrapper, String, ?]] | ||
Monad[EitherT[ListWrapper, String, ?]] | ||
MonadTrans[EitherT[?[_], String, ?]] | ||
|
||
// Tests for catsDataMonadErrorForEitherT, for recovery on errors on Either. | ||
checkAll("EitherT[ListWrapper, String, Int]", MonadErrorTests[EitherT[ListWrapper, String, ?], String].monadError[Int, Int, Int]) | ||
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! |
||
// Tests for MonadTrans instance. | ||
checkAll("EitherT[ListWrapper, String, Int]]", MonadTransTests[EitherT[?[_], String, ?]].monadTrans[ListWrapper, Int, Int]) | ||
checkAll("MonadTrans[EitherT[?[_], String, ?]]", SerializableTests.serializable(MonadTrans[EitherT[?[_], String, ?]])) | ||
} | ||
|
||
{ | ||
//if a Monad is defined | ||
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. just noticed here, should it be `if a MonadError is defined"? |
||
// Tests for catsDataMonadErrorFForEitherT instance, for recovery on errors of F. | ||
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. Minor, but should we say "if a MonadError is defined"? |
||
|
||
implicit val eq1 = EitherT.catsDataEqForEitherT[Option, String, Either[Unit, String]] | ||
implicit val eq2 = EitherT.catsDataEqForEitherT[EitherT[Option, String, ?], Unit, String](eq1) | ||
implicit val me = EitherT.catsDataMonadErrorFForEitherT[Option, Unit, String](catsStdInstancesForOption) | ||
|
||
Functor[EitherT[Option, String, ?]] | ||
Applicative[EitherT[Option, String, ?]] | ||
Monad[EitherT[Option, String, ?]] | ||
|
||
checkAll("EitherT[Option, String, String]", MonadErrorTests[EitherT[Option, String, ?], Unit].monadError[String, String, String]) | ||
checkAll("MonadError[EitherT[Option, ?, ?]]", SerializableTests.serializable(MonadError[EitherT[Option, String, ?], Unit])) | ||
} | ||
|
||
{ | ||
//if a Monad is defined | ||
implicit val F = ListWrapper.monad | ||
|
@@ -220,11 +239,6 @@ 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("transform consistent with value.map") { | ||
forAll { (eithert: EitherT[List, String, Int], f: Either[String, Int] => Either[Long, Double]) => | ||
eithert.transform(f) should === (EitherT(eithert.value.map(f))) | ||
|
@@ -340,6 +354,18 @@ class EitherTTests extends CatsSuite { | |
} | ||
} | ||
|
||
test("collectRight with Option consistent with flattening a to[Option]") { | ||
forAll { (et: EitherT[Option, String, Int]) => | ||
et.collectRight should === (et.to[Option].flatten) | ||
} | ||
} | ||
|
||
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.
nitpick: would you add a comment doc here to indicate that this instance delegate to the
MonadError
ofF
instead of usingEither
?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.
@kailuowang sure, I'm on it.