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 OptionT combinators for effectful Boolean #4390

Merged
merged 3 commits into from
Feb 3, 2023

Conversation

ivan-klass
Copy link
Contributor

  • OptionT[F, A].filterF(p: A => F[Boolean]) to filter on effectful F[Boolean]
  • OptionT.whenM[F[_], A](fp: F[Boolean])(a: => F[A]) to construct based on effectful F[Boolean]

Comment on lines +770 to +772
def whenM[F[_], A](cond: F[Boolean])(fa: => F[A])(implicit F: Monad[F]): OptionT[F, A] = OptionT(
F.ifM(cond)(ifTrue = F.map(fa)(Some(_)), ifFalse = F.pure(None))
)
Copy link
Contributor

@satorg satorg Feb 3, 2023

Choose a reason for hiding this comment

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

Just to note: although there's nothing particularly wrong with this method, however I think it might not be as useful as it might seem at first glance. Just because it does not allow monads chaining. I.e. if a chain of F[_] ends with F[Boolean] then the entire chain has to be enwrapped into whenF to lift the result into OptionT.

I think, a better approach could be adding an extension method for F[Boolean] itself. And there's a good place for it already:
mouse / fboolean.scala. E.g. it could be a new extension method of type

F[Boolean] => F[A] => F[Option[A]]

then there's another method liftOptionT in there mouse / foption.scala
that can also be applied aferwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

we already have when and whenF. In my personal opinion, I think it is fine to add it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, no objections either.

johnynek
johnynek previously approved these changes Feb 3, 2023
@johnynek
Copy link
Contributor

johnynek commented Feb 3, 2023

I went ahead and approved, but the optimization of avoiding the Some(a) reallocation is nice and I think we should get that in before merging.

satorg
satorg previously approved these changes Feb 3, 2023
Copy link
Contributor

@satorg satorg left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@ivan-klass ivan-klass dismissed stale reviews from satorg and johnynek via deea535 February 3, 2023 07:52
@armanbilge armanbilge changed the title Add OptionT combinators for effectful Boolean Add OptionT combinators for effectful Boolean Feb 3, 2023
@johnynek johnynek merged commit 50f8f6b into typelevel:main Feb 3, 2023
@johnynek
Copy link
Contributor

johnynek commented Feb 3, 2023

Thanks for this PR!

@ivan-klass ivan-klass deleted the OptionT-effectful-bools branch February 16, 2023 22:28
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.

4 participants