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 MonadError instance for EitherT that recovers from F[_] errors. #1644

Merged
merged 23 commits into from
Jul 24, 2017
Merged

Add MonadError instance for EitherT that recovers from F[_] errors. #1644

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
43eab48
Added map and recover F functions
leandrob13 Apr 30, 2017
979d869
Added tests for new EitherT functions
leandrob13 May 1, 2017
4148d21
Removed tests with Future in EitherTTests
leandrob13 May 1, 2017
ca76937
Merge branch 'master' of https://github.com/typelevel/cats into recov…
leandrob13 Jun 4, 2017
aa6a955
Added addtional MonadError instance for EitherT for recovering from F…
leandrob13 Jun 4, 2017
ab37c2b
Added transformF and deleted mapF
leandrob13 Jun 4, 2017
a09fe4c
Changed implicit parameter names in recoverF and recoverFWith
leandrob13 Jun 4, 2017
6d41f14
Added tests for EitherT collectRight and applyAlt
leandrob13 Jun 5, 2017
3882f81
Added MonadError tests for EitherT instance that recovers from F
leandrob13 Jun 6, 2017
5eae9e4
Removed MonadError tests with Future, replaced it with Option
leandrob13 Jun 6, 2017
c554233
Added serializable test
leandrob13 Jun 8, 2017
f477698
Changed the right type from Int to String in MonadError test for EitherT
leandrob13 Jun 8, 2017
eee4315
added option -Xlog-implicits to build.sbt
leandrob13 Jun 10, 2017
cd04677
Removed transformF from EitherT
leandrob13 Jun 10, 2017
3160827
Removed unused imports
leandrob13 Jun 10, 2017
d10318b
Redefined Eq instances in EitherTTests
leandrob13 Jun 11, 2017
b7e1760
Redo of the collectRight fix in EitherTTests
leandrob13 Jun 12, 2017
e7b5590
Removed unnecesary syntax import in EitherT tests
leandrob13 Jun 17, 2017
eabb6f2
Deleted recoverF and recoverFWith functions from EitherT
leandrob13 Jun 24, 2017
bdc44a8
Conflict resolution
leandrob13 Jul 3, 2017
b5425bd
Added comment doc for catsDataMonadErrorFForEitherT
leandrob13 Jul 3, 2017
abce798
Separated EitherT test for F recovery to another block
leandrob13 Jul 9, 2017
839f1be
Fixed comment in EitherT tests
leandrob13 Jul 22, 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
13 changes: 13 additions & 0 deletions core/src/main/scala/cats/data/EitherT.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,9 +483,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] =
Copy link
Contributor

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 of F instead of using Either?

Copy link
Contributor Author

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.

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
Expand Down Expand Up @@ -547,6 +551,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(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) {
Expand Down
26 changes: 20 additions & 6 deletions tests/src/test/scala/cats/tests/EitherTTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -46,17 +47,23 @@ 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)
implicit val eq2 = EitherT.catsDataEqForEitherT[Option, String, Either[Unit, String]]
implicit val eq3 = EitherT.catsDataEqForEitherT[EitherT[Option, String, ?], Unit, String](eq2)
implicit val me = EitherT.catsDataMonadErrorFForEitherT[Option, Unit, String](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])
Copy link
Contributor

Choose a reason for hiding this comment

The 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 catsDataMonadErrorFForEitherT and the above one is testing the catsDataMonadErrorForEitherT?

checkAll("MonadError[EitherT[List, ?, ?]]", SerializableTests.serializable(MonadError[EitherT[ListWrapper, String, ?], String]))
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to law tests the new instance.

Copy link
Contributor Author

@leandrob13 leandrob13 Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should separate these MonadError tests using Option in a separate section.

{
  // If a MonadError is defined for F
  val eq = ...
  
  checkAll("EitherT[Option, String, String]", ...)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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])
}

Expand Down Expand Up @@ -219,11 +226,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)))
Expand Down Expand Up @@ -339,6 +341,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)
Expand Down