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

MonadError instance for Ior #1548

Merged
merged 13 commits into from
Apr 9, 2017
Merged

MonadError instance for Ior #1548

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
344e232
Added MonadError instance for Ior
leandrob13 Mar 8, 2017
564c4ec
Revert "Added MonadError instance for Ior"
leandrob13 Mar 19, 2017
0e17151
Merge branch 'master' into iorMonadError
leandrob13 Mar 19, 2017
0f77e85
Added MonadError instance for Ior without mixing Traverse
leandrob13 Mar 19, 2017
b72d741
Removed Monad instance
leandrob13 Mar 19, 2017
43077d5
Added MonadError tests to Ior
leandrob13 Mar 19, 2017
213452a
Added MonadErrorTests for Ior
leandrob13 Mar 19, 2017
ededf7a
Revert "Added MonadErrorTests for Ior"
leandrob13 Mar 19, 2017
6c58070
Separated Traverse instance from ApplicativeError instance for Validated
leandrob13 Mar 19, 2017
a48b04c
Added test for ior valueOr
leandrob13 Mar 20, 2017
9ead46f
Conflict resolution
leandrob13 Apr 7, 2017
b2d3e46
Fixed some slastyle comments
leandrob13 Apr 7, 2017
d23e374
Deleted duplicated import
leandrob13 Apr 8, 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
60 changes: 41 additions & 19 deletions core/src/main/scala/cats/data/Ior.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package cats
package data

import cats.functor.Bifunctor

import scala.annotation.tailrec

/** Represents a right-biased disjunction that is either an `A`, or a `B`, or both an `A` and a `B`.
Expand Down Expand Up @@ -153,26 +154,42 @@ private[data] sealed abstract class IorInstances extends IorInstances0 {
def combine(x: Ior[A, B], y: Ior[A, B]) = x.append(y)
}

implicit def catsDataMonadForIor[A: Semigroup]: Monad[A Ior ?] = new Monad[A Ior ?] {
def pure[B](b: B): A Ior B = Ior.right(b)
def flatMap[B, C](fa: A Ior B)(f: B => A Ior C): A Ior C = fa.flatMap(f)
def tailRecM[B, C](b: B)(fn: B => Ior[A, Either[B, C]]): A Ior C = {
@tailrec
def loop(v: Ior[A, Either[B, C]]): A Ior C = v match {
case Ior.Left(a) => Ior.left(a)
case Ior.Right(Right(c)) => Ior.right(c)
case Ior.Both(a, Right(c)) => Ior.both(a, c)
case Ior.Right(Left(b)) => loop(fn(b))
case Ior.Both(a, Left(b)) =>
fn(b) match {
case Ior.Left(aa) => Ior.left(Semigroup[A].combine(a, aa))
case Ior.Both(aa, x) => loop(Ior.both(Semigroup[A].combine(a, aa), x))
case Ior.Right(x) => loop(Ior.both(a, x))
}
implicit def catsDataMonadErrorForIor[A: Semigroup]: MonadError[Ior[A, ?], A] =
new MonadError[Ior[A, ?], A] {

def raiseError[B](e: A): Ior[A, B] = Ior.left(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not tested


def handleErrorWith[B](fa: Ior[A, B])(f: (A) => Ior[A, B]): Ior[A, B] =
fa match {
case Ior.Left(e) => f(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not tested

case r @ Ior.Right(_) => r
case Ior.Both(e, _) => f(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not tested

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 Thanks for pointing them out. I will get it fixed this week.

}

def flatMap[B, C](fa: Ior[A, B])(f: B => Ior[A, C]): Ior[A, C] = fa.flatMap(f)

def tailRecM[B, C](b: B)(fn: B => Ior[A, Either[B, C]]): A Ior C = {
@tailrec
def loop(v: Ior[A, Either[B, C]]): A Ior C = v match {
case Ior.Left(a) => Ior.left(a)
case Ior.Right(Right(c)) => Ior.right(c)
case Ior.Both(a, Right(c)) => Ior.both(a, c)
case Ior.Right(Left(b)) => loop(fn(b))
case Ior.Both(a, Left(b)) =>
fn(b) match {
case Ior.Left(aa) => Ior.left(Semigroup[A].combine(a, aa))
case Ior.Both(aa, x) => loop(Ior.both(Semigroup[A].combine(a, aa), x))
case Ior.Right(x) => loop(Ior.both(a, x))
}
}
loop(fn(b))
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why you decided to merge this into the MonadError instance, but this one does not require A having a Semigroup instance. So if we don't have good reason, we should probably keep as is.

Copy link
Contributor Author

@leandrob13 leandrob13 Mar 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@kailuowang wouldn't I need it when I define a type like this:

type IorNel[A, B] = Ior[NonEmptyList[A], B]

If I try to use the traverse instance? I saw the mixin of Traverse and ApplicativeError on Validated so I thought it would be equivalent, or in this case is not relevant? I am considering this since I already have a PR #1540 with a proposition on that IorNel.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to keep. Traverse should not require a Semigroup.

Copy link
Contributor

Choose a reason for hiding this comment

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

@leandrob13 for a IorNel whether to require the left side being a Semigroup is irrelevant because the left side NonEmptyList is one. But there are other types of Ior in which the left side is not a Semigroup. We don't want to add unnecessary requirements/restriction to our instances.
I think the mixin in Validated is not optimal either in the exact same sense. The Traverse and Functor shouldn't need a Semigroup. Care to fix that one as well?

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 understood! I will fix the Validated instance as well. Thanks for the feedback.

}
loop(fn(b))

override def pure[B](x: B): Ior[A, B] = Ior.right(x)

override def map[B, C](fa: A Ior B)(f: B => C): A Ior C =
fa.map(f)
}
}

implicit def catsDataBifunctorForIor: Bifunctor[Ior] =
new Bifunctor[Ior] {
Expand All @@ -182,13 +199,18 @@ private[data] sealed abstract class IorInstances extends IorInstances0 {

private[data] sealed abstract class IorInstances0 {

implicit def catsDataTraverseFunctorForIor[A]: Traverse[A Ior ?] with Functor[A Ior ?] = new Traverse[A Ior ?] with Functor[A Ior ?] {
implicit def catsDataTraverseFunctorForIor[A]: Traverse[A Ior ?] = new Traverse[A Ior ?] {
def traverse[F[_]: Applicative, B, C](fa: A Ior B)(f: B => F[C]): F[A Ior C] =
fa.traverse(f)
def foldLeft[B, C](fa: A Ior B, b: C)(f: (C, B) => C): C =
fa.foldLeft(b)(f)
def foldRight[B, C](fa: A Ior B, lc: Eval[C])(f: (B, Eval[C]) => Eval[C]): Eval[C] =
fa.foldRight(lc)(f)

override def forall[B](fa: Ior[A, B])(p: (B) => Boolean): Boolean = fa.forall(p)

override def exists[B](fa: Ior[A, B])(p: (B) => Boolean): Boolean = fa.exists(p)

override def map[B, C](fa: A Ior B)(f: B => C): A Ior C =
fa.map(f)
}
Expand Down
73 changes: 41 additions & 32 deletions core/src/main/scala/cats/data/Validated.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,16 +295,8 @@ private[data] sealed abstract class ValidatedInstances extends ValidatedInstance
}

// scalastyle:off method.length
implicit def catsDataInstancesForValidated[E](implicit E: Semigroup[E]): Traverse[Validated[E, ?]] with ApplicativeError[Validated[E, ?], E] =
new Traverse[Validated[E, ?]] with ApplicativeError[Validated[E, ?], E] {
def traverse[F[_]: Applicative, A, B](fa: Validated[E, A])(f: A => F[B]): F[Validated[E, B]] =
fa.traverse(f)

def foldLeft[A, B](fa: Validated[E, A], b: B)(f: (B, A) => B): B =
fa.foldLeft(b)(f)

def foldRight[A, B](fa: Validated[E, A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
fa.foldRight(lb)(f)
implicit def catsDataApplicativeErrorForValidated[E](implicit E: Semigroup[E]): ApplicativeError[Validated[E, ?], E] =
new ApplicativeError[Validated[E, ?], E] {

def pure[A](a: A): Validated[E, A] =
Validated.valid(a)
Expand All @@ -324,6 +316,45 @@ private[data] sealed abstract class ValidatedInstances extends ValidatedInstance
case v @ Validated.Valid(_) => v
}
def raiseError[A](e: E): Validated[E, A] = Validated.Invalid(e)
}
// scalastyle:on method.length
}

private[data] sealed abstract class ValidatedInstances1 extends ValidatedInstances2 {

implicit def catsDataSemigroupForValidated[A, B](implicit A: Semigroup[A], B: Semigroup[B]): Semigroup[Validated[A, B]] =
new Semigroup[Validated[A, B]] {
def combine(x: Validated[A, B], y: Validated[A, B]): Validated[A, B] = x combine y
}

implicit def catsDataPartialOrderForValidated[A: PartialOrder, B: PartialOrder]: PartialOrder[Validated[A, B]] =
new PartialOrder[Validated[A, B]] {
def partialCompare(x: Validated[A, B], y: Validated[A, B]): Double = x partialCompare y
override def eqv(x: Validated[A, B], y: Validated[A, B]): Boolean = x === y
}
}

private[data] sealed abstract class ValidatedInstances2 {
implicit def catsDataEqForValidated[A: Eq, B: Eq]: Eq[Validated[A, B]] =
new Eq[Validated[A, B]] {
def eqv(x: Validated[A, B], y: Validated[A, B]): Boolean = x === y
}

// scalastyle:off method.length
implicit def catsDataTraverseFunctorForValidated[E]: Traverse[Validated[E, ?]] =
new Traverse[Validated[E, ?]] {

override def traverse[G[_] : Applicative, A, B](fa: Validated[E, A])(f: (A) => G[B]): G[Validated[E, B]] =
fa.traverse(f)

override def foldLeft[A, B](fa: Validated[E, A], b: B)(f: (B, A) => B): B =
fa.foldLeft(b)(f)

override def foldRight[A, B](fa: Validated[E, A], lb: Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
fa.foldRight(lb)(f)

override def map[A, B](fa: Validated[E, A])(f: (A) => B): Validated[E, B] =
fa.map(f)

override def reduceLeftToOption[A, B](fa: Validated[E, A])(f: A => B)(g: (B, A) => B): Option[B] =
fa.map(f).toOption
Expand Down Expand Up @@ -357,28 +388,6 @@ private[data] sealed abstract class ValidatedInstances extends ValidatedInstance

override def isEmpty[A](fa: Validated[E, A]): Boolean = fa.isInvalid
}
// scalastyle:on method.length
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to keep this // scalastyle:off method.length comment.

At the same time I think we can remove these comments around the ApplicativeError instance (catsDataApplicativeErrorForValidated).

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 I'm on it!

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 done with requested changes.

}

private[data] sealed abstract class ValidatedInstances1 extends ValidatedInstances2 {

implicit def catsDataSemigroupForValidated[A, B](implicit A: Semigroup[A], B: Semigroup[B]): Semigroup[Validated[A, B]] =
new Semigroup[Validated[A, B]] {
def combine(x: Validated[A, B], y: Validated[A, B]): Validated[A, B] = x combine y
}

implicit def catsDataPartialOrderForValidated[A: PartialOrder, B: PartialOrder]: PartialOrder[Validated[A, B]] =
new PartialOrder[Validated[A, B]] {
def partialCompare(x: Validated[A, B], y: Validated[A, B]): Double = x partialCompare y
override def eqv(x: Validated[A, B], y: Validated[A, B]): Boolean = x === y
}
}

private[data] sealed abstract class ValidatedInstances2 {
implicit def catsDataEqForValidated[A: Eq, B: Eq]: Eq[Validated[A, B]] =
new Eq[Validated[A, B]] {
def eqv(x: Validated[A, B], y: Validated[A, B]): Boolean = x === y
}
}

private[data] trait ValidatedFunctions {
Expand Down
16 changes: 12 additions & 4 deletions tests/src/test/scala/cats/tests/IorTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
package cats
package tests

import cats.data.Ior
import cats.data.{EitherT, Ior}
import cats.kernel.laws.GroupLaws
import cats.laws.discipline.{BifunctorTests, CartesianTests, MonadTests, SerializableTests, TraverseTests}
import cats.laws.discipline.{BifunctorTests, CartesianTests, MonadErrorTests, SerializableTests, TraverseTests}
import cats.laws.discipline.arbitrary._
import org.scalacheck.Arbitrary._

Expand All @@ -14,8 +14,10 @@ class IorTests extends CatsSuite {
checkAll("Ior[String, Int]", CartesianTests[Ior[String, ?]].cartesian[Int, Int, Int])
checkAll("Cartesian[String Ior ?]]", SerializableTests.serializable(Cartesian[String Ior ?]))

checkAll("Ior[String, Int]", MonadTests[String Ior ?].monad[Int, Int, Int])
checkAll("Monad[String Ior ?]]", SerializableTests.serializable(Monad[String Ior ?]))
implicit val eq0 = EitherT.catsDataEqForEitherT[Ior[String, ?], String, Int]

checkAll("Ior[String, Int]", MonadErrorTests[String Ior ?, String].monadError[Int, Int, Int])
checkAll("MonadError[String Ior ?]", SerializableTests.serializable(MonadError[String Ior ?, String]))

checkAll("Ior[String, Int] with Option", TraverseTests[String Ior ?].traverse[Int, Int, Int, Int, Option, Option])
checkAll("Traverse[String Ior ?]", SerializableTests.serializable(Traverse[String Ior ?]))
Expand Down Expand Up @@ -63,6 +65,12 @@ class IorTests extends CatsSuite {
}
}

test("valueOr consistent with leftMap") {
forAll { (i: Int Ior String, f: Int => String) =>
i.valueOr(f) should === (i.leftMap(f).fold(identity, identity, _ + _))
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding coverage.

test("isLeft consistent with toOption") {
forAll { (i: Int Ior String) =>
i.isLeft should === (i.toOption.isEmpty)
Expand Down