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.

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

Merged
merged 2 commits into from
Oct 23, 2018
Merged

Add biFlatMap to EitherT #2573

merged 2 commits into from
Oct 23, 2018

Conversation

agajek
Copy link
Contributor

@agajek agajek commented Oct 20, 2018

Fixes #2438

@codecov-io
Copy link

codecov-io commented Oct 20, 2018

Codecov Report

Merging #2573 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
core/src/main/scala/cats/data/EitherT.scala 97.9% <100%> (+0.04%) ⬆️

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 2f2a3b2...b16f5fa. Read the comment docs.

Copy link
Contributor

@ceedubs ceedubs left a 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] =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
@agajek
Copy link
Contributor Author

agajek commented Oct 21, 2018

@ceedubs is it possible to rerun travis' build? It indicates some memory problem of host rather than issues with my code.

@agajek
Copy link
Contributor Author

agajek commented Oct 22, 2018

@ceedubs, ready for merge. :)

@ceedubs
Copy link
Contributor

ceedubs commented Oct 22, 2018

We have a 2-approver rule for merging non-documentation PRs. @kailuowang or @LukaJCB would one of you want to take a look?

Copy link
Collaborator

@peterneyens peterneyens left a 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?

@LukaJCB LukaJCB merged commit 22b4ba7 into typelevel:master Oct 23, 2018
@ochrons
Copy link
Contributor

ochrons commented Oct 24, 2018

A question on naming, should this actually be biflatMap instead of biFlatMap as there are bimap and bitraverse with no capitalization in the middle? Although there is biSemiflatMap to confuse things further :)

@kailuowang kailuowang added this to the 1.5 milestone Oct 30, 2018
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
Copy link
Contributor

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants