Ior syntax #1540

leandrob13 · 2017-02-19T20:12:34Z

Added syntax for Ior like mentioned on issue #1539

This pull request includes ior syntax support for:

Validated
Option
Ior

…atatype

codecov-io · 2017-02-19T20:59:41Z

Codecov Report

Merging #1540 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1540      +/-   ##
=========================================
+ Coverage   92.48%   92.5%   +0.02%     
=========================================
  Files         246     247       +1     
  Lines        3885    3896      +11     
  Branches      133     129       -4     
=========================================
+ Hits         3593    3604      +11     
  Misses        292     292

Impacted Files	Coverage Δ
core/src/main/scala/cats/syntax/list.scala	`100% <ø> (ø)`	✅
core/src/main/scala/cats/data/package.scala	`83.33% <ø> (ø)`	✅
core/src/main/scala/cats/syntax/ior.scala	`100% <100%> (ø)`
core/src/main/scala/cats/syntax/option.scala	`100% <100%> (ø)`	✅
core/src/main/scala/cats/data/Ior.scala	`98.94% <100%> (+0.03%)`	✅
core/src/main/scala/cats/data/Validated.scala	`100% <100%> (ø)`	✅
core/src/main/scala/cats/data/Kleisli.scala	`92.59% <0%> (+0.09%)`	✅

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 ecdd27f...ef82db6. Read the comment docs.

ChristopherDavenport

In general I like the idea. I don't see any glaringly obvious problems.

ChristopherDavenport · 2017-02-20T14:53:03Z

tests/src/test/scala/cats/tests/ValidatedTests.scala

@@ -187,6 +187,18 @@ class ValidatedTests extends CatsSuite {
    }
  }

+  test("fromIor consistent with Ior.toValidated"){
+    forAll { (i: Ior[String, Int], s: String) =>


What is s used for here?

@ChristopherDavenport That just slipped, I will remove it. Thanks!

ChristopherDavenport · 2017-02-20T15:18:08Z

core/src/main/scala/cats/syntax/ior.scala

+    * res0: Ior[String, String] = Right(hello)
+    * }}}
+    */
+  def rightIor[B]: Ior[B, A] = Ior.right(a)


Context in Either instances uses asRight so perhaps asRightIor. Depends on what sort of syntax maintainers would like to see but asRightIor makes more sense to me.

It's only that way because of the Either.right projection ambiguity. The lack of such functions on Ior suggests to me that this way would be nicest.

leandrob13 · 2017-02-27T14:28:28Z

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

@@ -3,6 +3,7 @@ package cats
 package object data {
  type NonEmptyStream[A] = OneAnd[Stream, A]
  type ValidatedNel[+E, +A] = Validated[NonEmptyList[E], A]
+  type IorNel[+B, +A] = Ior[NonEmptyList[B], NonEmptyList[A]]


I am not sure about this definition now. Maybe it should be similar to ValidatedNel

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

I decided to define it with a NonEmptyList on the right also because I saw some utility on the combine case (or use of Ior's append function to be exact which can be accessed through semigroup syntax function |+|). But when comparing the combine for Either and Validated I think I got it wrong. Ior's flatmap does accumulate errors only if the case is Ior.Both, so it seems appropiate to leave it similar to ValidatedNel's type definition. I would love some feedback on this.

Personally I cannot see a use-case for IorNel as it was before; did you have one in mind? That might help to decide.

@edmundnoble I am going to change it to type IorNel[+B, +A] = Ior[NonEmptyList[B], A] to keep consistency with ValidatedNel. I do have a use case for my original proposal but that is when you get to combine Iors with the |+| operator. My main argument to change it is that in the flatMap you get tu accumulate errors on the left side of Both so it is the natural usage of it.

edmundnoble · 2017-02-20T22:37:16Z

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

@@ -47,6 +49,7 @@ sealed abstract class Ior[+A, +B] extends Product with Serializable {
  final def unwrap: Either[Either[A, B], (A, B)] = fold(a => Left(Left(a)), b => Left(Right(b)), (a, b) => Right((a, b)))

  final def toEither: Either[A, B] = fold(Left(_), Right(_), (_, b) => Right(b))
+  final def toValidated: Validated[A, B] = fold(Invalid(_), Valid(_), (_, b) => Valid(b))


edmundnoble · 2017-02-20T22:37:41Z

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

@@ -76,6 +76,11 @@ sealed abstract class Validated[+E, +A] extends Product with Serializable {
  def toOption: Option[A] = fold(_ => None, Some.apply)

  /**
+    * Returns Valid values wrapped in Ior.Right, and None for Ior.Left values
+    */
+  def toIor: Ior[E, A] = fold(Ior.left, Ior.right)


edmundnoble · 2017-02-20T22:38:44Z

core/src/main/scala/cats/syntax/ior.scala

+    * res0: Ior[String, String] = Right(hello)
+    * }}}
+    */
+  def rightIor[B]: Ior[B, A] = Ior.right(a)


It's only that way because of the Either.right projection ambiguity. The lack of such functions on Ior suggests to me that this way would be nicest.

edmundnoble · 2017-03-01T07:07:18Z

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

@@ -3,6 +3,7 @@ package cats
 package object data {
  type NonEmptyStream[A] = OneAnd[Stream, A]
  type ValidatedNel[+E, +A] = Validated[NonEmptyList[E], A]
+  type IorNel[+B, +A] = Ior[NonEmptyList[B], NonEmptyList[A]]


Personally I cannot see a use-case for IorNel as it was before; did you have one in mind? That might help to decide.

kailuowang · 2017-03-10T14:14:59Z

core/src/main/scala/cats/syntax/ior.scala

+    * res0: Ior[String, String] = Both(error,hello)
+    * }}}
+    */
+  def putRightIor[B](left: B): Ior[B, A] = Ior.both(left, a)


I am not sure about the necessity of these syntax:
putRightIor putLeftIor rightIorNel leftIorNel putRightIorNel putLeftIorNel
I am just not sure if the convenience of these methods overrides the cost of having them on everything.

@kailuowang in this case, I wanted to offer in the syntax the put functions that helped build Both type just for convenience. I really found useful the invalidNel syntax of validated so I wanted to be able to use something similar when I was working with Ior, that is why I added rightIorNel-leftIorNel. This means that you also object to Ior.rightNel-Ior.leftNel functions on the companion object?

I think you are right, rightIorNel leftIorNel look fine to me now.

kailuowang · 2017-03-10T14:19:06Z

core/src/main/scala/cats/syntax/ior.scala

+}
+
+
+final class IorNelListOps[A, B](val list: List[IorNel[A, B]]) extends AnyVal {


I think that this can be further abstracted. I am also not sure having syntax for such a specific type.

@kailuowang you mean that those function can be added to the Ior datatype?

I just feel this thing is too specific,
F[G[A, B]] => G[H[A], H[B]]. if F is a Functor and Reducable I feel like that want to see this function on a type class or companion obect

Maybe I am over thinking, maybe being able to reduce a list of Iors conveniently has good value. Generally speaking, I feel a good part of your PR can get easy consensus and this is a place I am less sure.

kailuowang · 2017-03-10T14:23:04Z

core/src/main/scala/cats/syntax/list.scala

+    * res1: IorNel[String, Int] = Right(1)
+    * }}}
+    */
+  def toIorNel[B](ifEmpty: => B): IorNel[A, B] =


The intention of this isn't very clear. I mean if you read a code result1.toIorNel(1), it's not obvious that the result1 is the error list while 1 is the valid result. Users will have to read the code here to understand the intention. I am not sure about such syntax addition.

@kailuowang you are right, I didn't make it clear. This is motivated due to a use case I find frequently. If there is some validations made and were returned as a List, meaning that if it is empty everything went well and if it is not then there are errors present, I had to do something like this:

errorList match { case Nil => Valid("this is valid") case err :: errs => Invalid(NoneEmptyList(err, errs)) }

I found convenient to have a syntax that helped me with this for ValidatedNel and IorNel.

I do agree with you that if there is not consensus on this I can roll it back and maybe try it again on another pull request when I can make an example illustrating the use of what I proposed. I'll wait for your thoughts on this.

kailuowang · 2017-03-10T14:23:17Z

core/src/main/scala/cats/syntax/list.scala

+    * }}}
+    */
+  def toValidatedNel[B](ifEmpty: => B): ValidatedNel[A, B] =
+    toNel.fold[ValidatedNel[A, B]](Valid(ifEmpty))(Invalid(_))


Same as above.

kailuowang · 2017-03-10T14:26:51Z

thanks very much @leandrob13, I like most of the PR, there are some syntax that I am not sure about. But I am more a minimalist when it comes to API design. So if other people see values in those syntax that I am not sure about I can be persuaded. Or if you want to separate those to another PR I'll gladly give my thumbs up on this one.

leandrob13 · 2017-03-10T16:03:25Z

@kailuowang I agree with you, my specific comments are above. Waiting for your feedback to decide what to do next.

… list syntax object

kailuowang

Thanks very much! 👍

kailuowang · 2017-03-10T16:59:19Z

core/src/main/scala/cats/syntax/ior.scala

+}
+
+
+final class IorNelListOps[A, B](val list: List[IorNel[A, B]]) extends AnyVal {


I just feel this thing is too specific,
F[G[A, B]] => G[H[A], H[B]]. if F is a Functor and Reducable I feel like that want to see this function on a type class or companion obect

kailuowang · 2017-03-10T17:07:30Z

core/src/main/scala/cats/syntax/ior.scala

+}
+
+
+final class IorNelListOps[A, B](val list: List[IorNel[A, B]]) extends AnyVal {


Maybe I am over thinking, maybe being able to reduce a list of Iors conveniently has good value. Generally speaking, I feel a good part of your PR can get easy consensus and this is a place I am less sure.

kailuowang · 2017-03-10T17:11:23Z

core/src/main/scala/cats/syntax/ior.scala

+    * res0: Ior[String, String] = Both(error,hello)
+    * }}}
+    */
+  def putRightIor[B](left: B): Ior[B, A] = Ior.both(left, a)


I think you are right, rightIorNel leftIorNel look fine to me now.

leandrob13 · 2017-03-11T02:16:40Z

@kailuowang Thanks! I will follow your advice and prepare another PR to illustrate better the proposals I made. I was actually thinking of a reduceable for the list of iors reduce proposal and thanks to your hint I might work on that too.

peterneyens · 2017-04-04T09:24:48Z

Merging with two 👍

leandrob13 added 9 commits February 18, 2017 23:42

Added Ior syntax

826b211

Removed examples on ior syntax comments

756d1ed

Removed toValidatedNel functions

4610791

Added syntax to list for IorNel and ValidatedNel

50bf4a6

Added test for toValidated function

3eb78f8

Added newline to end of ior.scala file

4350f57

Added toIor functions to option syntax

edf7466

Added fromIor to ValidatedFunctions and toIor function to Validated d…

2b18941

…atatype

Corrected error on duplicated implicit defs on syntax.ior.scala

54fb5c8

leandrob13 added 5 commits February 19, 2017 19:10

Added tests for new functions in Validated and Ior

831e32f

Added examples to list syntax functions

9e71a77

Corrected errors on list syntax examples

6924628

Added corrected examples to ior syntax

f03ab7d

Corrected names for IorNel syntax functions

989de6c

ChristopherDavenport reviewed Feb 20, 2017

View reviewed changes

leandrob13 mentioned this pull request Feb 27, 2017

MonadError instance for Ior #1544

Closed

leandrob13 commented Feb 27, 2017

View reviewed changes

edmundnoble approved these changes Mar 1, 2017

View reviewed changes

leandrob13 added 4 commits March 1, 2017 23:16

Redifined the IorNel type alias

0922773

Fixed test descriptions on IorTests

d650b09

Merge branch 'master' into iorSyntax

12f7e31

Merge branch 'master' into iorSyntax

b90a962

leandrob13 mentioned this pull request Mar 9, 2017

MonadError instance for Ior #1548

Merged

kailuowang reviewed Mar 10, 2017

View reviewed changes

Removed some functions from ior syntax object, deleted functions from…

ef82db6

… list syntax object

kailuowang approved these changes Mar 11, 2017

View reviewed changes

peterneyens merged commit e77bb99 into typelevel:master Apr 4, 2017

peterneyens mentioned this pull request Apr 4, 2017

Adding Ior syntax to cats.syntax package object #1539

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ior syntax #1540

Ior syntax #1540

leandrob13 commented Feb 19, 2017 •

edited

Loading

codecov-io commented Feb 19, 2017 •

edited

Loading

ChristopherDavenport left a comment

ChristopherDavenport Feb 20, 2017

leandrob13 Feb 20, 2017

ChristopherDavenport Feb 20, 2017 •

edited

Loading

edmundnoble Feb 20, 2017

leandrob13 Feb 27, 2017 •

edited

Loading

edmundnoble Mar 1, 2017

leandrob13 Mar 1, 2017

edmundnoble Feb 20, 2017

edmundnoble Feb 20, 2017

edmundnoble Feb 20, 2017

edmundnoble Mar 1, 2017

kailuowang Mar 10, 2017

leandrob13 Mar 10, 2017

kailuowang Mar 10, 2017

kailuowang Mar 10, 2017

leandrob13 Mar 10, 2017

kailuowang Mar 10, 2017

kailuowang Mar 10, 2017

kailuowang Mar 10, 2017

leandrob13 Mar 10, 2017

kailuowang Mar 10, 2017

kailuowang commented Mar 10, 2017

leandrob13 commented Mar 10, 2017

kailuowang left a comment

kailuowang Mar 10, 2017

kailuowang Mar 10, 2017

kailuowang Mar 10, 2017

leandrob13 commented Mar 11, 2017

peterneyens commented Apr 4, 2017

		}


		final class IorNelListOps[A, B](val list: List[IorNel[A, B]]) extends AnyVal {

Ior syntax #1540

Ior syntax #1540

Conversation

leandrob13 commented Feb 19, 2017 • edited Loading

codecov-io commented Feb 19, 2017 • edited Loading

Codecov Report

ChristopherDavenport left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristopherDavenport Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandrob13 Feb 27, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Mar 10, 2017

leandrob13 commented Mar 10, 2017

kailuowang left a 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 11, 2017

peterneyens commented Apr 4, 2017

leandrob13 commented Feb 19, 2017 •

edited

Loading

codecov-io commented Feb 19, 2017 •

edited

Loading

ChristopherDavenport Feb 20, 2017 •

edited

Loading

leandrob13 Feb 27, 2017 •

edited

Loading