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 2 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
11 changes: 11 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 @@ -88,11 +88,22 @@ 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 mapF[N[_], D](fe: F[Either[A, B]] => N[Either[A, D]]): EitherT[N, A, D] = EitherT(fe(value))
Copy link
Contributor

@kailuowang kailuowang May 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

this and the leftMapF feels weird to me. The type signature doesn't prevent the user from mapping changing the other side.

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 recognize your point. It is mapping F to N. Whatever is inside, could be a Left or a Right (mapped or not) and it is still mapping the Either. If I left the F[] to N[] without mapping the Either inside it would be a transform (F[] ~> N[]) but in EitherT transform is like

def transform[C, D](f: Either[A, B] => Either[C, D])(implicit F: Functor[F]): EitherT[F, C, D] =
    EitherT(F.map(value)(f))

Should it be a transformF?

def transformF[N[_], C, D](fe: F[Either[A, B]] => N[Either[C, D]]): EitherT[F, A, D] = 
  EitherT(fe(value))

Should we make standard what to do in a mapF? In Kleisli it looks like this:

def mapF[N[_], C](f: F[B] => N[C]): Kleisli[N, A, C] =
    Kleisli(run andThen f)

That is why I thought I could add a mapping function from F[] to N[] in EitherT. Also, transform in Kleisli means a transformation from F to N and also maps what is inside of F. Should we fix those semantics in the monad transformers?

Copy link
Contributor

@kailuowang kailuowang May 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This transformF sounds like we might need a new higher order type class? like

trait HFunctor[F[_[_], _] {
  def hmap[A[_], B[_], T](fa: F[A, T])(fk: A ~> B): F[B, T]
}

There is some discussion in this gist. It maybe too theoretical, but I think we can see its practial usage here.

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 will look into that!

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 taken your advice I opened #1713 and came to the conclusion to add this to EitherT

def transformF[G[_]](f: FunctionK[F, G]): EitherT[G, A, B] = EitherT(f(value))

There is something confusing about EitherT and OptionT using transform as a bimap/map of the parametrized types respectively. I called it transformF just to avoid the confusion. The issue is opened for the evaluation of your proposal and to address the confusion around transform semantics.


def recoverF[E](pf: PartialFunction[E, Either[A, B]])(implicit ae: ApplicativeError[F, E]): EitherT[F, A, B] =
Copy link
Contributor

Choose a reason for hiding this comment

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

This also feels weird to me because you can "recover" to the left side, which is a different semantics from the original recover. I suspect that these two different sets of recovers and recoverWiths should probably best be organized into two different derivations of MonadError instances. That way the code organization might be clearer and more importantly, we might be able to test with MonadError laws.

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 that is exactly what I intended, maybe the names should be changed so the semantics are right? The problem is that if you have an val x = EitherT(Future.failed(new Exception("Error"))) you can recover it with x.recoverFWith[Throwable](ex => Future.successfull(Either.left(ex.getMessage))) or if you want with a Right too. I didn't add a MonadError implicitly because Validated doesn't have one.

What do you mean by organizing them into two different derivations of MonadError instances?

As I discussed in #1643, there is a limitation on recovering for monad transformers like EitherT because you can't recover from F (my example is with a Future). My intention is to keep it practical and since EitherT has its own implementation of recover and recoverWith which are used for overriding the corresponding function of its MonadError instace, I thought we could add these functions without including them in MonadError or some other typeclass.

Copy link
Contributor

Choose a reason for hiding this comment

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

So as @djspiewak pointed out in #1643, there are two ways to provide a MonadError instance for a EitherT, the existing one ignores the error in F and recovers from the left side of the Either. We can have a new MonadError instance if theF already has MonadError instance and delegate to that, which is basically what you are doing with recoverF. These two instances might be able to coexist since the Error type is different, which should be true as long as the E in F: MonadError and the EitherT's left type are different, right?

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 right! Let me work it out then, thanks!

EitherT(ae.recover(value)(pf))

def recoverFWith[E](pf: PartialFunction[E, F[Either[A, B]]])(implicit ae: ApplicativeError[F, E]): EitherT[F, A, B] =
EitherT(ae.recoverWith(value)(pf))

def semiflatMap[D](f: B => F[D])(implicit F: Monad[F]): EitherT[F, A, D] =
flatMap(b => EitherT.right(f(b)))

def leftMap[C](f: A => C)(implicit F: Functor[F]): EitherT[F, C, B] = bimap(f, identity)

def leftMapF[N[_], C](fe: F[Either[A, B]] => N[Either[C, B]]): EitherT[N, C, B] =
EitherT(fe(value))

def compare(that: EitherT[F, A, B])(implicit o: Order[F[Either[A, B]]]): Int =
o.compare(value, that.value)

Expand Down
70 changes: 65 additions & 5 deletions tests/src/test/scala/cats/tests/EitherTTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
package cats
package tests

import cats.data.EitherT
import cats.data.{EitherT, Validated}
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 @@ -217,9 +218,68 @@ 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 in a Future") {
import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.duration._
import scala.concurrent.{Await, Future}

val et: EitherT[Future, String, Int] = EitherT.right[String](Future.failed(new Exception("error")))
Await.result(et.recoverF[Throwable] {
case ex => Either.right(5)
}.value, 10.seconds) should === (Right(5))
}

test("recoverF recovers handled values") {
val et: EitherT[Option, String, Int] = EitherT.right[String](Option.empty[Int])
et.recoverF[Unit] { case u => Right(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("recoverF ignores unhandled values") {
val et: EitherT[Validated[String, ?], String, Int] = EitherT.right[String](Validated.invalid("error"))
et.recoverF[String] { case "other" => Either.left("another") } should === (et)
}

test("recoverFWith recovers handled values in a Future") {
import scala.concurrent.ExecutionContext.Implicits.global
import scala.concurrent.duration._
import scala.concurrent.{Await, Future}

val et: EitherT[Future, String, Int] = EitherT.right[String](Future.failed(new Exception("error")))
Await.result(et.recoverFWith[Throwable] {
case ex => Future.successful(Right(5))
}.value, 10.seconds) should === (Right(5))
}

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("recoverFWith ignores unhandled values") {
val et: EitherT[Validated[String, ?], String, Int] = EitherT.right[String](Validated.invalid("error"))
et.recoverFWith[String] { case "other" => Validated.valid(Either.left("another")) } should === (et)
}

test("mapF consistent with mapping value") {
forAll { (eithert: EitherT[List, String, Int], f: List[Either[String, Int]] => Option[Either[String, String]]) =>
eithert.mapF(f).value should === (f(eithert.value))
}
}

test("leftMapF consistent with mapping value") {
forAll { (eithert: EitherT[List, String, Int], f: List[Either[String, Int]] => Option[Either[Int, Int]]) =>
eithert.leftMapF(f).value should === (f(eithert.value))
}
}

test("transform consistent with value.map") {
Expand Down