Add semiflatTap and leftSemiflatTap functions to EitherT

[matwojcik]

semiflatTaps functions on EitherT are highly useful when you have some e.g. Logger[F] abstractions and you don't want to lift it to EitherT[F,A,B]

[codecov-io]

Codecov Report

Merging #3316 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3316      +/-   ##
==========================================
+ Coverage    93.2%   93.42%   +0.22%     
==========================================
  Files         377      378       +1     
  Lines        7604     7631      +27     
  Branches      199      206       +7     
==========================================
+ Hits         7087     7129      +42     
+ Misses        517      502      -15

Flag	Coverage Δ
#scala_version_212	93.49% <100%> (?)
#scala_version_213	93.17% <100%> (-0.03%)	⬇️

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/EitherT.scala	98.5% <100%> (+0.01%)	⬆️
core/src/main/scala/cats/Show.scala	100% <0%> (ø)	⬆️
core/src/main/scala/cats/instances/tuple.scala	100% <0%> (ø)	⬆️
core/src/main/scala/cats/data/Const.scala	100% <0%> (ø)	⬆️
core/src/main/scala/cats/data/Validated.scala	100% <0%> (ø)	⬆️
core/src/main/scala/cats/package.scala	100% <0%> (ø)	⬆️
...cala-2.12/cats/kernel/compat/TraversableOnce.scala	0% <0%> (ø)
core/src/main/scala/cats/data/NonEmptyList.scala	98.73% <0%> (+0.01%)	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	99.25% <0%> (+0.01%)	⬆️
core/src/main/scala/cats/data/NonEmptySet.scala	97.64% <0%> (+0.02%)	⬆️
... 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 1ede81e...74cd724. Read the comment docs.

[travisbrown]

I like this addition, thanks!

This is a case where I'd personally be inclined to move the implementation down a couple of levels:

  def semiflatTap[C](f: B => F[C])(implicit F: Monad[F]): EitherT[F, A, B] =
    EitherT(F.flatMap(value) {
      case l @ Left(_) => F.pure(EitherUtil.rightCast(l))
      case Right(b)    => F.as(f(b), Right(b))
    })

This isn't quite as clear as your version, but in my view the difference in readability is pretty small, and it avoids a lot of unnecessary wrapping and unwrapping. I don't think we need to worry too much about optimization here, but if we can avoid some allocations more or less for free I think we should.

(For what it's worth I also think we should do the same thing for the existing semiflatMap and leftSemiflatMap implementations.)

[matwojcik]

@travisbrown I'm not fully convinced ;) In the end it uses flatMap which resembles your suggestion however it does indeed one extra wrapping and unwrapping.

So it adds one wrap/unwrap for a price of more readable code and additionally existing code is reused. Plus it isn't a method like flatMap/map which would be used all over again in the same stack, so the penalty should be low.

However I'm not that attached to the original solution, so can change it if you insist ;)

[travisbrown]

@matwojcik It's not a blocker for me. The signature is the important part, if someone wants to make an argument that we should change the implementations later they can.

[travisbrown]

The test failure was BigDecimal commutativity on 2.13, which is a known issue, so I'm restarting that job.

I have an only moderately controversial fix for that problem that's waiting for another sign-off: #3303

[LukaJCB]

What do we think about using Unit here to signify that this resulting type is going to be discarded? Might be a bit more cumbersome on the usage side, but it's just an extra .void

[travisbrown]

We already have a flatTap on FlatMap that doesn't require Unit, and tap, tapWith, etc. on Kleisli don't constrain the output type to be Unit. My personal preference would be consistency—I think that plus slightly easier use outweighs a little extra expressivity in the types.

[matwojcik]

@LukaJCB I first thought about it, but then saw that FlatMap[F].flatTap takes A => F[B] instead of A => F[Unit], so I wanted to be consistent with existing signature.