MonadError instance for Ior #1548

leandrob13 · 2017-03-08T05:50:49Z

codecov-io · 2017-03-08T06:27:55Z

Codecov Report

Merging #1548 into master will increase coverage by 0.67%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1548      +/-   ##
==========================================
+ Coverage   92.42%   93.09%   +0.67%     
==========================================
  Files         249      250       +1     
  Lines        3971     3983      +12     
  Branches      146      130      -16     
==========================================
+ Hits         3670     3708      +38     
+ Misses        301      275      -26

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/Validated.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/Ior.scala	`100% <100%> (+1.05%)`	⬆️
...aws/src/main/scala/cats/laws/ApplicativeLaws.scala	`100% <0%> (ø)`	⬆️
.../scala/cats/laws/discipline/ApplicativeTests.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/syntax/coproduct.scala
core/src/main/scala/cats/data/Coproduct.scala
core/src/main/scala/cats/data/Prod.scala
core/src/main/scala/cats/instances/bitSet.scala	`100% <0%> (ø)`
core/src/main/scala/cats/data/Tuple2K.scala	`100% <0%> (ø)`
core/src/main/scala/cats/data/EitherK.scala	`100% <0%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e77bb99...d23e374. Read the comment docs.

kailuowang · 2017-03-09T14:53:01Z

core/src/main/scala/cats/data/Ior.scala

+  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)


This line is not tested

kailuowang · 2017-03-09T14:53:06Z

core/src/main/scala/cats/data/Ior.scala

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


This line is not tested

kailuowang · 2017-03-09T14:53:10Z

core/src/main/scala/cats/data/Ior.scala

+        fa match {
+          case Ior.Left(e) => f(e)
+          case r @ Ior.Right(_) => r
+          case Ior.Both(e, _) => f(e)


This line is not tested

@kailuowang Thanks for pointing them out. I will get it fixed this week.

kailuowang · 2017-03-09T15:16:28Z

core/src/main/scala/cats/data/Ior.scala


  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 ?] {


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.

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

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

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

@kailuowang understood! I will fix the Validated instance as well. Thanks for the feedback.

This reverts commit 344e232.

This reverts commit 213452a.

kailuowang · 2017-03-20T15:23:49Z

tests/src/test/scala/cats/tests/IorTests.scala

+      i.valueOr(f) should === (i.leftMap(f).fold(identity, identity, _ + _))
+    }
+  }
+


Thanks for adding coverage.

kailuowang · 2017-03-20T15:26:20Z

👍 LKTM, thanks very much @leandrob13 for all the efforts to address the feedbacks!

leandrob13 · 2017-03-20T19:32:03Z

@kailuowang you are welcome. I have learned a lot.

peterneyens · 2017-04-04T13:54:26Z

core/src/main/scala/cats/data/Validated.scala

@@ -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


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

@peterneyens I'm on it!

@peterneyens done with requested changes.

peterneyens · 2017-04-07T14:56:19Z

tests/src/test/scala/cats/tests/IorTests.scala

 import cats.kernel.laws.GroupLaws
 import cats.laws.discipline.{BifunctorTests, CartesianTests, MonadErrorTests, SerializableTests, TraverseTests}
+import cats.data.{Ior, NonEmptyList, EitherT}
+import cats.kernel.laws.GroupLaws


You had this import already on line 4 it seems.

@peterneyens darn it. That one must have slipped when I merged. I'm on it.

Added MonadError instance for Ior

344e232

kailuowang reviewed Mar 9, 2017

View reviewed changes

leandrob13 mentioned this pull request Mar 9, 2017

Separate Traverse mixin on Validated instances #1553

Closed

leandrob13 added 9 commits March 18, 2017 19:21

Revert "Added MonadError instance for Ior"

564c4ec

This reverts commit 344e232.

Merge branch 'master' into iorMonadError

0e17151

Added MonadError instance for Ior without mixing Traverse

0f77e85

Removed Monad instance

b72d741

Added MonadError tests to Ior

43077d5

Added MonadErrorTests for Ior

213452a

Revert "Added MonadErrorTests for Ior"

ededf7a

This reverts commit 213452a.

Separated Traverse instance from ApplicativeError instance for Validated

6c58070

Added test for ior valueOr

a48b04c

kailuowang reviewed Mar 20, 2017

View reviewed changes

peterneyens reviewed Apr 4, 2017

View reviewed changes

leandrob13 added 2 commits April 6, 2017 19:16

Conflict resolution

9ead46f

Fixed some slastyle comments

b2d3e46

peterneyens reviewed Apr 7, 2017

View reviewed changes

Deleted duplicated import

d23e374

edmundnoble self-requested a review April 9, 2017 00:47

edmundnoble approved these changes Apr 9, 2017

View reviewed changes

peterneyens approved these changes Apr 9, 2017

View reviewed changes

peterneyens merged commit f73752c into typelevel:master Apr 9, 2017

kailuowang modified the milestone: 1.0.0-MF Apr 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MonadError instance for Ior #1548

MonadError instance for Ior #1548

leandrob13 commented Mar 8, 2017 •

edited

Loading

codecov-io commented Mar 8, 2017 •

edited

Loading

kailuowang Mar 9, 2017

kailuowang Mar 9, 2017

kailuowang Mar 9, 2017

leandrob13 Mar 9, 2017

kailuowang Mar 9, 2017

leandrob13 Mar 9, 2017 •

edited

Loading

johnynek Mar 9, 2017

kailuowang Mar 9, 2017

leandrob13 Mar 9, 2017

kailuowang Mar 20, 2017

kailuowang commented Mar 20, 2017

leandrob13 commented Mar 20, 2017

peterneyens Apr 4, 2017

leandrob13 Apr 4, 2017

leandrob13 Apr 7, 2017

peterneyens Apr 7, 2017

leandrob13 Apr 7, 2017

MonadError instance for Ior #1548

MonadError instance for Ior #1548

Conversation

leandrob13 commented Mar 8, 2017 • edited Loading

codecov-io commented Mar 8, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandrob13 Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Mar 20, 2017

leandrob13 commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandrob13 commented Mar 8, 2017 •

edited

Loading

codecov-io commented Mar 8, 2017 •

edited

Loading

leandrob13 Mar 9, 2017 •

edited

Loading