attemptTap

[RaasAhsan]

This is a common enough pattern that I think it deserves its own combinator. I defined it in MonadErrorSyntax for now, but could consider moving it to MonadError, unsure what the criteria for that is. You could possibly write a more efficient implementation depending on your F and I don't think it would break compatibility either.

[codecov-commenter]

Codecov Report

Merging #3459 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3459   +/-   ##
=======================================
  Coverage   91.99%   91.99%           
=======================================
  Files         383      383           
  Lines        8372     8374    +2     
  Branches      212      216    +4     
=======================================
+ Hits         7702     7704    +2     
  Misses        670      670           

[djspiewak]

I'm not sure exactly what common pattern this is replacing? I guess exactly what the implementation is doing?

[RaasAhsan]

We already have an analogue for Future#onSuccess/Future#foreach (FlatMap#flatTap) and Future#onError (ApplicativeError#onError). This would be the pure analogue to Future#onComplete, which is useful for use cases like reporting metrics or logging. This function makes call chains a bit more compact.

fa
  .attempt
  .flatTap {
    case Right(a) => log.info(s"succeeded with $a")
    case Left(e) => log.error(s"failed with $e")
  }
  .rethrow

// Ironically this one has fewer LOC but harder to factor out the bodies
fa
  .flatTap(a => log.info(s"succeeded with $a"))
  .onError(e => log.error(s"failed with $e"))

fa.attemptTap {
  case Right(a) => log.info(s"succeeded with $a")
  case Left(e) => log.error(s"failed with $e")
}

aside: I could see an argument for accepting f as a PartialFunction

[djspiewak]

Ah I see what you mean. Alrighty, I'm broadly 👍 on this then. Re your aside, I think the total function is better since Scala's PartialFunction/Function1 inheritance is backwards, so this is the more flexible arrangement.

[travisbrown]

I'm 👍 on the general idea, but is there a reason not to put this directly on MonadError and let the syntax be generated by simulacrum-scalafix (i.e. like the attempt, flatTap, and rethrow methods it composes)?

[travisbrown]

Also it'd be good to add a usage example test (like in e.g. #3467).

[RaasAhsan]

@travisbrown MonadError and ApplicativeError don't seem to be annotated with @typeclass, so no syntax is generated, which I'm guessing is why they're all manually defined in the cats.syntax.monadError package. Should they be?

Added in an example, not very happy about the println but it demonstrates that some effect happens

[LukaJCB]

@RaasAhsan, simulacrum can't handle multi-param typeclasses, that's why we have them manually defined :)

[barambani]

just a note, but don't consider it a blocker for the PR. I personally like the example in your comment here

fa.attemptTap {
  case Right(a) => log.info(s"succeeded with $a")
  case Left(e) => log.error(s"failed with $e")
}

It describes very well a real use case for attemptTap. A simple program could be simulated with State. Something like

/**
 * {{{
 * scala> import cats._, data._, implicits._
 * scala> type F[A] = EitherT[State[String, *], String, A]
 * scala> object log { def info(m: String): F[Unit] = EitherT.liftF(State.modify(_ => s"info: $m")); def error(m: String): F[Unit] = EitherT.liftF(State.modify(_ => s"error: $m")) }
 *
 * scala> val prog1: F[Int] = EitherT.liftF(State.pure(1))
 * scala> val prog2: F[Int] = "this one failed".raiseError[F, Int]
 *
 * scala> val run1 = prog1.attemptTap { case Right(a) => log.info(a.toString); case Left(e) => log.error(e) }
 * scala> val run2 = prog2.attemptTap { case Right(a) => log.info(a.toString); case Left(e) => log.error(e) }
 *
 * scala> run1.value.runS("init").value
 * res0: String = info: 1
 *
 * scala> run2.value.runS("init").value
 * res1: String = error: this one failed
 * }}}
 */

[travisbrown]

Looks good to me!