Implement EitherT#leftFlatMap and EitherT#leftSemiflatMap #1790

vendethiel · 2017-08-06T09:48:27Z

Modeled after the right versions.

codecov-io · 2017-08-06T10:18:50Z

Codecov Report

Merging #1790 into master will increase coverage by 0.58%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1790      +/-   ##
==========================================
+ Coverage   94.96%   95.55%   +0.58%     
==========================================
  Files         241      248       +7     
  Lines        4193     4451     +258     
  Branches      115      118       +3     
==========================================
+ Hits         3982     4253     +271     
+ Misses        211      198      -13

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/EitherT.scala	`98.36% <100%> (+0.08%)`	⬆️
kernel/src/main/scala/cats/kernel/Order.scala	`86.48% <0%> (-2.71%)`	⬇️
core/src/main/scala/cats/Eval.scala	`98.75% <0%> (-1.25%)`	⬇️
core/src/main/scala/cats/Monad.scala	`96% <0%> (-0.16%)`	⬇️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`92.06% <0%> (-0.13%)`	⬇️
core/src/main/scala/cats/data/NonEmptyList.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/instances/list.scala	`100% <0%> (ø)`	⬆️
...ain/scala/cats/laws/discipline/TraverseTests.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/instances/order.scala	`100% <0%> (ø)`	⬆️
...ree/src/main/scala/cats/free/FreeApplicative.scala	`100% <0%> (ø)`	⬆️
... and 33 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 61d553d...edef908. Read the comment docs.

kailuowang · 2017-08-07T14:45:24Z

thanks very much! 👍

johnynek · 2017-08-11T17:44:13Z

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

+    })
+
+  def leftSemiflatMap[D](f: A => F[D])(implicit F: Monad[F]): EitherT[F, D, B] =
+    leftFlatMap(a => EitherT.left(f(a)))


this is going to allocate the EitherT just to call .value above. Why not just inline the implementation:

EitherT(F.flatMap(value) { case Left(a) => f(a).map { d => Left(d) } case r@Right(_) => F.pure(r.leftCast) })

vendethiel · 2017-08-28T13:35:49Z

@johnynek Back from holidays. Did just that.

jrduncans · 2017-08-29T01:40:13Z

Seems like there should be a leftFlatMap for f: (A) ⇒ F[Either[D, BB]] to fully match flatMap.

vendethiel · 2017-08-29T09:35:35Z

You mean leftFlatMapF?

jrduncans · 2017-08-29T12:43:08Z

Oh, yes, that's correct.

non · 2017-08-30T15:24:33Z

I'd like to have tests for these in terms of swap and the right-biased methods, e.g.

et.leftFlatMap(a => f(a)) <-> et.swap.flatMap(a => f(a).swap)

Does that seem reasonable?

vendethiel · 2017-08-30T15:52:14Z

@non I tried to do it at first, but I missed one swap. It seems to work with one extra:

et.leftFlatMap(a => f(a)) <-> et.swap.flatMap(a => f(a).swap).swap

Should I leave the previous test there and add this one, or scratch the one I currently have?
And should I try to write a leftSemiflatMap in terms of semiflatMap as well?
Should I add leftFlatMapF?

non · 2017-08-31T02:22:33Z

@vendethiel Let's add these sorts of tests in addition to what you already have. I'd go ahead and write tests like this for all the left-biased methods that have a right-biased equivalent.

vendethiel · 2017-08-31T08:19:22Z

That means 2 foralls per method.

Okay, what about leftFlatMapF, do we want that?

LukaJCB · 2017-09-03T10:19:05Z

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

+  def leftFlatMap[BB >: B, D](f: A => EitherT[F, D, BB])(implicit F: Monad[F]): EitherT[F, D, BB] =
+    EitherT(F.flatMap(value) {
+      case Left(a) => f(a).value
+      case r@Right(_) => F.pure(r.leftCast)


Tiny nitpick, but can we change this to case r: Right => ... since we don't actually want to extract anything?

While in practice I don't think it matters, the use of : implies type casing which is generally used in contexts where parametricity is violated: https://typelevel.org/blog/2014/11/10/why_is_adt_pattern_matching_allowed.html

I could have sworn there were some cases where : was allowed and ADT patmat wasn't but I can't remember.

Very interesting article, thanks a lot! :)
I always thought that scalac transforms case t: Type into an isInstanceOf check whereas case Type(_) would be turned into Type.unapply(...), but that article shows that my assumption was false.
It seems then, there's no real benefit to my suggestion, so disregard.

LukaJCB · 2017-09-03T10:19:16Z

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

+  def leftSemiflatMap[D](f: A => F[D])(implicit F: Monad[F]): EitherT[F, D, B] =
+    EitherT(F.flatMap(value) {
+      case Left(a) => F.map(f(a)) { d => Left(d) }
+      case r@Right(_) => F.pure(r.leftCast)


Ditto, as above :)

kailuowang · 2017-09-25T17:57:24Z

@vendethiel if you waiting for an answer for leftFlatMapF I'd say let's address that in a different PR.

vendethiel · 2017-09-25T18:41:54Z

Okay, thanks. Then I think the PR is ready?

kailuowang · 2017-09-25T18:47:33Z

I still think the tests @non suggested would be valuable to add.

LukaJCB

Tiny note, just about naming of tests, otherwise LGTM :)

LukaJCB · 2017-09-26T11:12:06Z

tests/src/test/scala/cats/tests/EitherTTests.scala

@@ -434,4 +434,22 @@ class EitherTTests extends CatsSuite {
    } yield s1 ++ s2
  }

+  test("leftFlatMap") {
+    forAll { (eithert: EitherT[List, String, Int], f: String => String) =>


Could we name this one "leftFlatMap consistent with leftMap"?

LukaJCB · 2017-09-26T11:12:39Z

tests/src/test/scala/cats/tests/EitherTTests.scala

+    forAll { (eithert: EitherT[List, String, Int], f: String => String) =>
+      eithert.leftFlatMap(v => EitherT.left[Int](List(f(v)))) should ===(eithert.leftMap(f))
+    }
+    forAll { (eithert: EitherT[List, String, Int], f: String => EitherT[List, String, Int]) =>


How about naming this one "leftFlatMap consistent with swap and then flatMap"?

LukaJCB · 2017-09-26T11:13:10Z

tests/src/test/scala/cats/tests/EitherTTests.scala

+    }
+  }
+
+  test("leftSemiflatMap") {


Same as above for these two tests :)

Tiny nit

vendethiel · 2017-09-29T08:19:25Z

Somehow missed the notifications! Sorry.

LukaJCB · 2017-09-29T08:20:02Z

Don't be, it's all good! Thanks very much for your contribution! 😊

kailuowang · 2017-09-29T12:58:18Z

👍 Thanks very much indeed!

vendethiel · 2017-09-29T13:08:42Z

Thanks! 🍰

kailuowang modified the milestone: 1.0.0-RC1 Aug 8, 2017

johnynek reviewed Aug 11, 2017

View reviewed changes

Implement EitherT#leftFlatMap and EitherT#leftSemiflatMap

cd38406

vendethiel force-pushed the feature/either-leftflatmap-leftsemiflatmap branch from 8de4070 to cd38406 Compare August 28, 2017 13:35

LukaJCB reviewed Sep 3, 2017

View reviewed changes

Add additional tests for EitherT

9c27cf3

LukaJCB reviewed Sep 26, 2017

View reviewed changes

Rename tests

edef908

Tiny nit

LukaJCB approved these changes Sep 29, 2017

View reviewed changes

kailuowang merged commit 66b4ac7 into typelevel:master Sep 29, 2017

vendethiel deleted the feature/either-leftflatmap-leftsemiflatmap branch September 29, 2017 13:08

kailuowang added the enhancement label Oct 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement EitherT#leftFlatMap and EitherT#leftSemiflatMap #1790

Implement EitherT#leftFlatMap and EitherT#leftSemiflatMap #1790

vendethiel commented Aug 6, 2017

codecov-io commented Aug 6, 2017 •

edited

Loading

kailuowang commented Aug 7, 2017

johnynek Aug 11, 2017

vendethiel commented Aug 28, 2017

jrduncans commented Aug 29, 2017

vendethiel commented Aug 29, 2017

jrduncans commented Aug 29, 2017

non commented Aug 30, 2017 •

edited

Loading

vendethiel commented Aug 30, 2017 •

edited

Loading

non commented Aug 31, 2017

vendethiel commented Aug 31, 2017 •

edited

Loading

LukaJCB Sep 3, 2017

adelbertc Sep 4, 2017

LukaJCB Sep 4, 2017

LukaJCB Sep 3, 2017

kailuowang commented Sep 25, 2017

vendethiel commented Sep 25, 2017

kailuowang commented Sep 25, 2017 •

edited

Loading

LukaJCB left a comment

LukaJCB Sep 26, 2017

LukaJCB Sep 26, 2017

LukaJCB Sep 26, 2017

vendethiel commented Sep 29, 2017

LukaJCB commented Sep 29, 2017

kailuowang commented Sep 29, 2017

vendethiel commented Sep 29, 2017

Implement EitherT#leftFlatMap and EitherT#leftSemiflatMap #1790

Implement EitherT#leftFlatMap and EitherT#leftSemiflatMap #1790

Conversation

vendethiel commented Aug 6, 2017

codecov-io commented Aug 6, 2017 • edited Loading

Codecov Report

kailuowang commented Aug 7, 2017

Choose a reason for hiding this comment

vendethiel commented Aug 28, 2017

jrduncans commented Aug 29, 2017

vendethiel commented Aug 29, 2017

jrduncans commented Aug 29, 2017

non commented Aug 30, 2017 • edited Loading

vendethiel commented Aug 30, 2017 • edited Loading

non commented Aug 31, 2017

vendethiel commented Aug 31, 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

kailuowang commented Sep 25, 2017

vendethiel commented Sep 25, 2017

kailuowang commented Sep 25, 2017 • edited Loading

LukaJCB 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

vendethiel commented Sep 29, 2017

LukaJCB commented Sep 29, 2017

kailuowang commented Sep 29, 2017

vendethiel commented Sep 29, 2017

codecov-io commented Aug 6, 2017 •

edited

Loading

non commented Aug 30, 2017 •

edited

Loading

vendethiel commented Aug 30, 2017 •

edited

Loading

vendethiel commented Aug 31, 2017 •

edited

Loading

kailuowang commented Sep 25, 2017 •

edited

Loading