-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Add biFlatMap to EitherT #2573
Add biFlatMap to EitherT #2573
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2573 +/- ##
==========================================
+ Coverage 95.14% 95.14% +<.01%
==========================================
Files 361 361
Lines 6630 6633 +3
Branches 289 284 -5
==========================================
+ Hits 6308 6311 +3
Misses 322 322
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @agajek! You can slightly relax the Monad
constraint, but after you make that change, this looks good to me.
@@ -77,6 +77,13 @@ final case class EitherT[F[_], A, B](value: F[Either[A, B]]) { | |||
applicativeG: Applicative[G]): G[EitherT[F, C, D]] = | |||
applicativeG.map(traverseF.traverse(value)(axb => Bitraverse[Either].bitraverse(axb)(f, g)))(EitherT.apply) | |||
|
|||
def biFlatMap[AA >: A, BB >: B](fa: A => EitherT[F, AA, BB], | |||
fb: B => EitherT[F, AA, BB])(implicit F: Monad[F]): EitherT[F, AA, BB] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fb: B => EitherT[F, AA, BB])(implicit F: Monad[F]): EitherT[F, AA, BB] = | |
fb: B => EitherT[F, AA, BB])(implicit F: FlatMap[F]): EitherT[F, AA, BB] = |
Relaxed constraint of the Monad to FlatMap in biFlatMap Fixes #2438
@ceedubs is it possible to rerun travis' build? It indicates some memory problem of host rather than issues with my code. |
@ceedubs, ready for merge. :) |
We have a 2-approver rule for merging non-documentation PRs. @kailuowang or @LukaJCB would one of you want to take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @agajek.
Maybe using EitherT.leftT
and EitherT.rightT
would have been a little bit simpler in the tests?
A question on naming, should this actually be |
fb: B => EitherT[F, AA, BB])(implicit F: FlatMap[F]): EitherT[F, AA, BB] = | ||
EitherT(F.flatMap(value) { | ||
case Left(a) => fa(a).value | ||
case Right(a) => fb(a).value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would not it be more intuitive to say case Right(b) => fb(b).value
at L.84 to match with the type B
fb expects?
Fixes #2438