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

Changes from 1 commit
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
83 changes: 50 additions & 33 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 @@ -140,7 +140,7 @@ object Ior extends IorInstances with IorFunctions {
final case class Both[+A, +B](a: A, b: B) extends (A Ior B)
}

private[data] sealed abstract class IorInstances extends IorInstances0 {
private[data] sealed abstract class IorInstances {
implicit def catsDataEqForIor[A: Eq, B: Eq]: Eq[A Ior B] = new Eq[A Ior B] {
def eqv(x: A Ior B, y: A Ior B): Boolean = x === y
}
Expand All @@ -153,47 +153,64 @@ 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))
}
// scalastyle:off method.length
implicit def catsDataMonadErrorForIor[A: Semigroup]: MonadError[Ior[A, ?], A] with Traverse[Ior[A, ?]] =
new MonadError[Ior[A, ?], A] with Traverse[Ior[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)

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 map[B, C](fa: A Ior B)(f: B => C): A Ior C =
fa.map(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)
}
}
// scalastyle:on method.length

implicit def catsDataBifunctorForIor: Bifunctor[Ior] =
new Bifunctor[Ior] {
override def bimap[A, B, C, D](fab: A Ior B)(f: A => C, g: B => D): C Ior D = fab.bimap(f, g)
}
}

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 ?] {
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 map[B, C](fa: A Ior B)(f: B => C): A Ior C =
fa.map(f)
}
}

private[data] sealed trait IorFunctions {
def left[A, B](a: A): A Ior B = Ior.Left(a)
def right[A, B](b: B): A Ior B = Ior.Right(b)
Expand Down