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

Partially revert 4530d3aa93131ec28096968a3c903ed1016dbf1b. Add back v… #1506

Merged
merged 4 commits into from
Jan 3, 2017
Merged

Partially revert 4530d3aa93131ec28096968a3c903ed1016dbf1b. Add back v… #1506

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d084def
Partially revert 4530d3aa93131ec28096968a3c903ed1016dbf1b. Add back v…
Dec 29, 2016
5c86c44
Add missing tests for Either syntax
marcin-rzeznicki Dec 30, 2016
95d96fa
Fix the docs to match added variance to Either's enriched `flatMap`. …
Dec 30, 2016
5cafcdd
Resurrect deleted section about `Either`
marcin-rzeznicki Dec 31, 2016
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
21 changes: 11 additions & 10 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 @@ -21,19 +21,19 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {

def swap(implicit F: Functor[F]): EitherT[F, B, A] = EitherT(F.map(value)(_.swap))

def getOrElse(default: => B)(implicit F: Functor[F]): F[B] = F.map(value)(_.getOrElse(default))
def getOrElse[BB >: B](default: => BB)(implicit F: Functor[F]): F[BB] = F.map(value)(_.getOrElse(default))

def getOrElseF(default: => F[B])(implicit F: Monad[F]): F[B] = {
def getOrElseF[BB >: B](default: => F[BB])(implicit F: Monad[F]): F[BB] = {
F.flatMap(value) {
case Left(_) => default
case Right(b) => F.pure(b)
}
}

def orElse(default: => EitherT[F, A, B])(implicit F: Monad[F]): EitherT[F, A, B] = {
def orElse[AA, BB >: B](default: => EitherT[F, AA, BB])(implicit F: Monad[F]): EitherT[F, AA, BB] = {
EitherT(F.flatMap(value) {
case Left(_) => default.value
case r @ Right(_) => F.pure(r)
case r @ Right(_) => F.pure(r.leftCast)
})
}

Expand All @@ -46,13 +46,14 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {
case other => F.pure(other)
})

def valueOr(f: A => B)(implicit F: Functor[F]): F[B] = fold(f, identity)
def valueOr[BB >: B](f: A => BB)(implicit F: Functor[F]): F[BB] = fold(f, identity)

def forall(f: B => Boolean)(implicit F: Functor[F]): F[Boolean] = F.map(value)(_.forall(f))

def exists(f: B => Boolean)(implicit F: Functor[F]): F[Boolean] = F.map(value)(_.exists(f))

def ensure(onFailure: => A)(f: B => Boolean)(implicit F: Functor[F]): EitherT[F, A, B] = EitherT(F.map(value)(_.ensure(onFailure)(f)))
def ensure[AA >: A](onFailure: => AA)(f: B => Boolean)(implicit F: Functor[F]): EitherT[F, AA, B] =
EitherT(F.map(value)(_.ensure(onFailure)(f)))

def toOption(implicit F: Functor[F]): OptionT[F, B] = OptionT(F.map(value)(_.toOption))

Expand All @@ -70,19 +71,19 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {
def applyAlt[D](ff: EitherT[F, A, B => D])(implicit F: Apply[F]): EitherT[F, A, D] =
EitherT[F, A, D](F.map2(this.value, ff.value)((xb, xbd) => Apply[Either[A, ?]].ap(xbd)(xb)))

def flatMap[D](f: B => EitherT[F, A, D])(implicit F: Monad[F]): EitherT[F, A, D] =
def flatMap[AA >: A, D](f: B => EitherT[F, AA, D])(implicit F: Monad[F]): EitherT[F, AA, D] =
EitherT(F.flatMap(value) {
case l @ Left(_) => F.pure(l.rightCast)
case Right(b) => f(b).value
})

def flatMapF[D](f: B => F[Either[A, D]])(implicit F: Monad[F]): EitherT[F, A, D] =
def flatMapF[AA >: A, D](f: B => F[Either[AA, D]])(implicit F: Monad[F]): EitherT[F, AA, D] =
flatMap(f andThen EitherT.apply)

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

def subflatMap[D](f: B => Either[A, D])(implicit F: Functor[F]): EitherT[F, A, D] =
def subflatMap[AA >: A, D](f: B => Either[AA, D])(implicit F: Functor[F]): EitherT[F, AA, D] =
transform(_.flatMap(f))

def map[D](f: B => D)(implicit F: Functor[F]): EitherT[F, A, D] = bimap(identity, f)
Expand Down Expand Up @@ -110,7 +111,7 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) {
def foldRight[C](lc: Eval[C])(f: (B, Eval[C]) => Eval[C])(implicit F: Foldable[F]): Eval[C] =
F.foldRight(value, lc)((axb, lc) => axb.foldRight(lc)(f))

def merge(implicit ev: B <:< A, F: Functor[F]): F[A] = F.map(value)(_.fold(identity, ev.apply))
def merge[AA >: A](implicit ev: B <:< AA, F: Functor[F]): F[AA] = F.map(value)(_.fold(identity, ev.apply))

/**
* Similar to `Either#combine` but mapped over an `F` context.
Expand Down
50 changes: 25 additions & 25 deletions core/src/main/scala/cats/syntax/either.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,27 +23,27 @@ final class EitherOps[A, B](val eab: Either[A, B]) extends AnyVal {
case Right(b) => f(b)
}

def getOrElse(default: => B): B = eab match {
def getOrElse[BB >: B](default: => BB): BB = eab match {
case Left(_) => default
case Right(b) => b
}

def orElse[C](fallback: => Either[C, B]): Either[C, B] = eab match {
def orElse[C, BB >: B](fallback: => Either[C, BB]): Either[C, BB] = eab match {
case Left(_) => fallback
case r @ Right(_) => EitherUtil.leftCast(r)
}

def recover(pf: PartialFunction[A, B]): Either[A, B] = eab match {
def recover[BB >: B](pf: PartialFunction[A, BB]): Either[A, BB] = eab match {
case Left(a) if pf.isDefinedAt(a) => Right(pf(a))
case _ => eab
}

def recoverWith(pf: PartialFunction[A, Either[A, B]]): Either[A, B] = eab match {
def recoverWith[AA >: A, BB >: B](pf: PartialFunction[A, Either[AA, BB]]): Either[AA, BB] = eab match {
case Left(a) if pf.isDefinedAt(a) => pf(a)
case _ => eab
}

def valueOr(f: A => B): B = eab match {
def valueOr[BB >: B](f: A => BB): BB = eab match {
case Left(a) => f(a)
case Right(b) => b
}
Expand All @@ -58,7 +58,7 @@ final class EitherOps[A, B](val eab: Either[A, B]) extends AnyVal {
case Right(b) => f(b)
}

def ensure(onFailure: => A)(f: B => Boolean): Either[A, B] = eab match {
def ensure[AA >: A](onFailure: => AA)(f: B => Boolean): Either[AA, B] = eab match {
case Left(_) => eab
case Right(b) => if (f(b)) eab else Left(onFailure)
}
Expand Down Expand Up @@ -90,7 +90,7 @@ final class EitherOps[A, B](val eab: Either[A, B]) extends AnyVal {

/** Returns a [[cats.data.ValidatedNel]] representation of this disjunction with the `Left` value
* as a single element on the `Invalid` side of the [[cats.data.NonEmptyList]]. */
def toValidatedNel: ValidatedNel[A, B] = eab match {
def toValidatedNel[AA >: A]: ValidatedNel[AA, B] = eab match {
case Left(a) => Validated.invalidNel(a)
case Right(b) => Validated.valid(b)
}
Expand All @@ -113,7 +113,7 @@ final class EitherOps[A, B](val eab: Either[A, B]) extends AnyVal {
case Right(b) => Right(f(b))
}

def map2Eval[C, Z](fc: Eval[Either[A, C]])(f: (B, C) => Z): Eval[Either[A, Z]] =
def map2Eval[AA >: A, C, Z](fc: Eval[Either[AA, C]])(f: (B, C) => Z): Eval[Either[AA, Z]] =
eab match {
case l @ Left(_) => Now(EitherUtil.rightCast(l))
case Right(b) => fc.map(either => new EitherOps(either).map(f(b, _)))
Expand All @@ -124,51 +124,51 @@ final class EitherOps[A, B](val eab: Either[A, B]) extends AnyVal {
case r @ Right(_) => EitherUtil.leftCast(r)
}

def flatMap[D](f: B => Either[A, D]): Either[A, D] = eab match {
def flatMap[AA >: A, D](f: B => Either[AA, D]): Either[AA, D] = eab match {
case l @ Left(_) => EitherUtil.rightCast(l)
case Right(b) => f(b)
}

def compare(that: Either[A, B])(implicit A: Order[A], B: Order[B]): Int = eab match {
def compare[AA >: A, BB >: B](that: Either[AA, BB])(implicit AA: Order[AA], BB: Order[BB]): Int = eab match {
case Left(a1) =>
that match {
case Left(a2) => A.compare(a1, a2)
case Left(a2) => AA.compare(a1, a2)
case Right(_) => -1
}
case Right(b1) =>
that match {
case Left(_) => 1
case Right(b2) => B.compare(b1, b2)
case Right(b2) => BB.compare(b1, b2)
}
}

def partialCompare(that: Either[A, B])(implicit A: PartialOrder[A], B: PartialOrder[B]): Double = eab match {
def partialCompare[AA >: A, BB >: B](that: Either[AA, BB])(implicit AA: PartialOrder[AA], BB: PartialOrder[BB]): Double = eab match {
Copy link
Contributor

Choose a reason for hiding this comment

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

this method is untested. Is it too much to ask to write a test of some kind during this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at all. Will do

Copy link
Contributor Author

@marcin-rzeznicki marcin-rzeznicki Dec 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Actually this one should be covered by order laws at https://github.com/typelevel/cats/blob/master/tests/src/test/scala/cats/tests/EitherTests.scala#L44 , right?

EDIT: Ah, it isn't because implementation of partialCompare in EitherInstances is duplicated in EitherOps. Is it better to dedup these two?

case Left(a1) =>
that match {
case Left(a2) => A.partialCompare(a1, a2)
case Left(a2) => AA.partialCompare(a1, a2)
case Right(_) => -1
}
case Right(b1) =>
that match {
case Left(_) => 1
case Right(b2) => B.partialCompare(b1, b2)
case Right(b2) => BB.partialCompare(b1, b2)
}
}

def ===(that: Either[A, B])(implicit A: Eq[A], B: Eq[B]): Boolean = eab match {
def ===[AA >: A, BB >: B](that: Either[AA, BB])(implicit AA: Eq[AA], BB: Eq[BB]): Boolean = eab match {
case Left(a1) =>
that match {
case Left(a2) => A.eqv(a1, a2)
case Left(a2) => AA.eqv(a1, a2)
case Right(_) => false
}
case Right(b1) =>
that match {
case Left(_) => false
case Right(b2) => B.eqv(b1, b2)
case Right(b2) => BB.eqv(b1, b2)
}
}

def traverse[F[_], D](f: B => F[D])(implicit F: Applicative[F]): F[Either[A, D]] = eab match {
def traverse[F[_], AA >: A, D](f: B => F[D])(implicit F: Applicative[F]): F[Either[AA, D]] = eab match {
case l @ Left(_) => F.pure(EitherUtil.rightCast(l))
case Right(b) => F.map(f(b))(Right(_))
}
Expand Down Expand Up @@ -215,20 +215,20 @@ final class EitherOps[A, B](val eab: Either[A, B]) extends AnyVal {
* res3: Either[String, Int] = Right(7)
* }}}
*/
final def combine(that: Either[A, B])(implicit B: Semigroup[B]): Either[A, B] = eab match {
final def combine[AA >: A, BB >: B](that: Either[AA, BB])(implicit BB: Semigroup[BB]): Either[AA, BB] = eab match {
case left @ Left(_) => left
case Right(b1) => that match {
case left @ Left(_) => left
case Right(b2) => Right(B.combine(b1, b2))
case Right(b2) => Right(BB.combine(b1, b2))
}
}

def show(implicit A: Show[A], B: Show[B]): String = eab match {
case Left(a) => s"Left(${A.show(a)})"
case Right(b) => s"Right(${B.show(b)})"
def show[AA >: A, BB >: B](implicit AA: Show[AA], BB: Show[BB]): String = eab match {
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. Can we add a simple test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course

case Left(a) => s"Left(${AA.show(a)})"
case Right(b) => s"Right(${BB.show(b)})"
}

def ap[C](that: Either[A, B => C]): Either[A, C] = (new EitherOps(that)).flatMap(this.map)
def ap[AA >: A, BB >: B, C](that: Either[AA, BB => C]): Either[AA, C] = new EitherOps(that).flatMap(this.map)
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. Can we add a simple test?


/**
* Transform the `Either` into a [[cats.data.EitherT]] while lifting it into the specified Applicative.
Expand Down
126 changes: 0 additions & 126 deletions docs/src/main/tut/datatypes/either.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,132 +212,6 @@ magic("123") match {
}
```

## Either in the small, Either in the large
Once you start using `Either` for all your error-handling, you may quickly run into an issue where
you need to call into two separate modules which give back separate kinds of errors.

```tut:silent
sealed abstract class DatabaseError
trait DatabaseValue

object Database {
def databaseThings(): Either[DatabaseError, DatabaseValue] = ???
}

sealed abstract class ServiceError
trait ServiceValue

object Service {
def serviceThings(v: DatabaseValue): Either[ServiceError, ServiceValue] = ???
}
```

Let's say we have an application that wants to do database things, and then take database
values and do service things. Glancing at the types, it looks like `flatMap` will do it.

```scala
def doApp = Database.databaseThings().flatMap(Service.serviceThings)
```

If you're on Scala 2.12, this line will compile and work as expected, but if you're on an earlier
version of Scala it won't! This difference is related to the right-biasing of `Either` in Scala 2.12
that was mentioned above. In Scala 2.12 the `flatMap` we get here is a method on `Either` with this
signature:

```scala
def flatMap[AA >: A, Y](f: (B) => Either[AA, Y]): Either[AA, Y]
```

This `flatMap` is different from the ones you'll find on `List` or `Option`, for example, in that it
has two type parameters, with the extra `AA` parameter allowing us to `flatMap` into an `Either`
with a different type on the left side.

This behavior is consistent with the covariance of `Either`, and in some cases it can be convenient,
but it also makes it easy to run into nasty variance issues (such as `Object` being inferred as the
type of the left side, as it is in this case).

For this reason the `flatMap` provided by Cats's `Either` syntax (which is the one you'll get for
Scala 2.10 and 2.11) does not include this extra type parameter. Instead the left sides have to
match, which means our `doApp` definition above will not compile on versions of Scala before 2.12.

### Solution 1: Application-wide errors
So clearly in order for us to easily compose `Either` values, the left type parameter must be the same.
We may then be tempted to make our entire application share an error data type.

```tut:silent
sealed abstract class AppError
final case object DatabaseError1 extends AppError
final case object DatabaseError2 extends AppError
final case object ServiceError1 extends AppError
final case object ServiceError2 extends AppError

trait DatabaseValue

object Database {
def databaseThings(): Either[AppError, DatabaseValue] = ???
}

object Service {
def serviceThings(v: DatabaseValue): Either[AppError, ServiceValue] = ???
}

def doApp = Database.databaseThings().flatMap(Service.serviceThings)
```

This certainly works, or at least it compiles. But consider the case where another module wants to just use
`Database`, and gets an `Either[AppError, DatabaseValue]` back. Should it want to inspect the errors, it
must inspect **all** the `AppError` cases, even though it was only intended for `Database` to use
`DatabaseError1` or `DatabaseError2`.

### Solution 2: ADTs all the way down
Instead of lumping all our errors into one big ADT, we can instead keep them local to each module, and have
an application-wide error ADT that wraps each error ADT we need.

```tut:silent
sealed abstract class DatabaseError
trait DatabaseValue

object Database {
def databaseThings(): Either[DatabaseError, DatabaseValue] = ???
}

sealed abstract class ServiceError
trait ServiceValue

object Service {
def serviceThings(v: DatabaseValue): Either[ServiceError, ServiceValue] = ???
}

sealed abstract class AppError
object AppError {
final case class Database(error: DatabaseError) extends AppError
final case class Service(error: ServiceError) extends AppError
}
```

Now in our outer application, we can wrap/lift each module-specific error into `AppError` and then
call our combinators as usual. `Either` provides a convenient method to assist with this, called `Either.leftMap` -
it can be thought of as the same as `map`, but for the `Left` side.

```tut:silent
def doApp: Either[AppError, ServiceValue] =
Database.databaseThings().leftMap[AppError](AppError.Database).
flatMap(dv => Service.serviceThings(dv).leftMap(AppError.Service))
```

Hurrah! Each module only cares about its own errors as it should be, and more composite modules have their
own error ADT that encapsulates each constituent module's error ADT. Doing this also allows us to take action
on entire classes of errors instead of having to pattern match on each individual one.

```tut:silent
def awesome =
doApp match {
case Left(AppError.Database(_)) => "something in the database went wrong"
case Left(AppError.Service(_)) => "something in the service went wrong"
case Right(_) => "everything is alright!"
}
```

## Working with exception-y code
Copy link
Contributor

Choose a reason for hiding this comment

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

Please be more careful about removing this section - variance does not subsume the concerns here.

There will inevitably come a time when your nice `Either` code will have to interact with exception-throwing
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this being removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@adelbertc The idea is that all the stuff about the left sides needing to be the same will be a lot less relevant (or not at all relevant, in the form here) once the syntax flatMap for Either behaves the same as the real flatMap method on Either in 2.12.

Copy link
Contributor

Choose a reason for hiding this comment

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

This section talks about using local ADTs for local errors and wrapping as you go wider and wider in your application. Variance is tangentially relevant to Solution 1 but does not help with Solution 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, you've convinced me the removal should be more surgical. :)

Copy link
Contributor Author

@marcin-rzeznicki marcin-rzeznicki Dec 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Nonetheless, the whole section was presented as a 'solution' - but the problem being solved does not exist anymore. To quote the relevant part: 'If you're on Scala 2.12, this line will compile and work as expected (...)'. Everything that followed seemed to have been written as a guide to deal with issues on previous versions of Scala. It of course does not invalidate ideas contained therein. Obviously, organizing your errors as ADTs is a valid and useful technique. But a rather generic one and not really tied to usage of Either in cats. Of course I'm open to any suggestions, but I must admit that I simply do not know what should be there instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should strive to present certain patterns (for lack of a better word) for using these data types - in this case I wrote it to show how one could use Either throughout their code to represent possibly failing computations.

code. Handling such situations is easy enough.
Expand Down
25 changes: 25 additions & 0 deletions tests/src/test/scala/cats/tests/EitherTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,4 +255,29 @@ class EitherTests extends CatsSuite {
x.to[Option] should === (x.toOption)
}
}

test("partialCompare consistent with PartialOrder") {
forAll { (x: Either[Int, String], y: Either[Int, String]) =>
x.partialCompare(y) should === (partialOrder.partialCompare(x, y))
}
}

test("show Right") {
val either = Either.right[String, Int](10)
either.show should === ("Right(10)")
}

test("show Left") {
val either = Either.left[String, Int]("string")
either.show should === ("Left(string)")
}

test("ap consistent with Applicative") {
val fab = implicitly[Applicative[Either[String, ?]]]
forAll { (fa: Either[String, Int],
f: Int => String) =>
fa.ap(Either.right(f)) should === (fab.map(fa)(f))
}
}

}