Applicative whenA/unlessA syntax extensions should respect call-by-name parameter #3899
Conversation
| @@ -17,3 +19,8 @@ final class ApplicativeOps[F[_], A](private val fa: F[A]) extends AnyVal { | |||
| def unlessA(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.unlessA(cond)(fa) | |||
There was a problem hiding this comment.
I also suggest deprecating these two
There was a problem hiding this comment.
You could just make the old syntax not implicit and substitute the new one with the same names.
There was a problem hiding this comment.
@joroKr21 then replicateA will become a syntax extenstion on => F[A], while the method itself accepts F[A]. Is it ok?
There was a problem hiding this comment.
@catostrophe how about replicateA(0)? Shouldn't that also not execute fa?
There was a problem hiding this comment.
Yes, but replicateA in Applicative takes a call-by-value param
There was a problem hiding this comment.
Yes, and exposing a by-name API for replicateA might mislead users to think of a more imperative style of API like List.fill where => F[A] might have side-effects.
Another option would be to extract ApplicativeStrictOps for replicateA
| @@ -17,3 +19,8 @@ final class ApplicativeOps[F[_], A](private val fa: F[A]) extends AnyVal { | |||
| def unlessA(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.unlessA(cond)(fa) | |||
| def whenA(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.whenA(cond)(fa) | |||
| } | |||
|
|
|||
| final class ApplicativeByNameOps[F[_], A](private val fa: () => F[A]) extends AnyVal { | |||
| def unlessA_(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.unlessA(cond)(fa()) | |||
There was a problem hiding this comment.
I think we should use .void on these two.
There was a problem hiding this comment.
sorry, I got confused. Often we have foo_ as a variant of foo that discards the value. But I see that here we are doing something else (just using it as another version that uses call by name correctly.
I would like to see another name since, I think, that pattern is established and this would be a violation of it.
There was a problem hiding this comment.
🐑 Um, if you think of a naming variant convention for "like X but call-by-name" we'd like to use it in mouse too, similar situation
There was a problem hiding this comment.
I offer suffix L for Lazy, thus whenAL & unlessAL. It's short, and the reason for the L is at least sane, although not immediately self-evident
| @@ -17,3 +19,8 @@ final class ApplicativeOps[F[_], A](private val fa: F[A]) extends AnyVal { | |||
| def unlessA(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.unlessA(cond)(fa) | |||
| def whenA(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.whenA(cond)(fa) | |||
| } | |||
|
|
|||
| final class ApplicativeByNameOps[F[_], A](private val fa: () => F[A]) extends AnyVal { | |||
There was a problem hiding this comment.
Perhaps a minor thing, but wouldn't it be better to keep constructors of *-Ops classes private to the syntax package? I know, not all the existing classes do follow it, but perhaps it is a good idea for all new classes, wdyt?
final class ApplicativeByNameOps[F[_], A] private[syntax] (private val fa: () => F[A])
extends AnyValI mean, it is unlikely that we want to allow users to create Ops files explicitly, isn't? Or is there some use case for that?
The current implementation of the
.whenA/.unlessAsyntax extensions doesn't respectF[A]being passed as call-by-name.This leads to issues where it's important.
See: typelevel/fs2#2412
Unfortunately we can't change whenA/unlessA directly due to bincompat guarantees.
But the new method names even better reflect that we throw
Aout (as intraverse_). I would also deprecate the old ones.Closes #3687