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.

Sign up for GitHub

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

Jump to bottom

Applicative whenA/unlessA syntax extensions should respect call-by-name parameter #3899

Merged
merged 1 commit into from
May 23, 2022
Merged

Applicative whenA/unlessA syntax extensions should respect call-by-name parameter #3899

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions core/src/main/scala/cats/syntax/applicative.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ trait ApplicativeSyntax {
new ApplicativeIdOps[A](a)
implicit final def catsSyntaxApplicative[F[_], A](fa: F[A]): ApplicativeOps[F, A] =
new ApplicativeOps[F, A](fa)
implicit final def catsSyntaxApplicativeByName[F[_], A](fa: => F[A]): ApplicativeByNameOps[F, A] =
new ApplicativeByNameOps[F, A](() => fa)
}

final class ApplicativeIdOps[A](private val a: A) extends AnyVal {
Expand All @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also suggest deprecating these two

Copy link
Member

Choose a reason for hiding this comment

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

You could just make the old syntax not implicit and substitute the new one with the same names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joroKr21 then replicateA will become a syntax extenstion on => F[A], while the method itself accepts F[A]. Is it ok?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm I'm not sure

Copy link
Member

Choose a reason for hiding this comment

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

@catostrophe how about replicateA(0)? Shouldn't that also not execute fa?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but replicateA in Applicative takes a call-by-value param

Copy link
Member

Choose a reason for hiding this comment

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

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

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 {
Copy link
Contributor

@satorg satorg Nov 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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 AnyVal

I 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?

def unlessA_(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.unlessA(cond)(fa())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use .void on these two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, what for?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

🐑 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

Copy link
Member

Choose a reason for hiding this comment

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

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

def whenA_(cond: Boolean)(implicit F: Applicative[F]): F[Unit] = F.whenA(cond)(fa())
}