Add foldF, cataF and emptyflatTap to OptionT by ybasket · Pull Request #3335 · typelevel/cats

ybasket · 2020-02-29T15:45:57Z

Adds several handy methods to OptionT:

foldF allows to fold effectually, similar to EitherT.foldF
cataF is a variant of foldF with one parameter list, similar as cata is to fold
emptyflatTap (suggestions for better names welcome) allows to perform an effect if the OptionT carries a None value inside, leaving the result unchanged (except if raising an error, that would propagate).

An example for a use case for emptyflatTap is looking up a value in a cache (returning F[Option[V]] which you wrap in OptionT for further handling, but logging (as F[Unit]) if the value was empty:

val cachedResult: F[Option[V]] = cache.get(...)
OptionT(cachedResult)
  .emptyflatTap(logger.info("Cache was empty"))
  .doMoreStuffOnOptionT

codecov-io · 2020-02-29T18:30:11Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #3335   +/-   ##
=======================================
  Coverage   92.70%   92.71%           
=======================================
  Files         379      379           
  Lines        7981     7985    +4     
  Branches      230      230           
=======================================
+ Hits         7399     7403    +4     
  Misses        582      582

Flag	Coverage Δ
#scala_version_212	`92.74% <100.00%> (+<0.01%)`	⬆️
#scala_version_213	`92.50% <100.00%> (+<0.01%)`	⬆️

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/OptionT.scala	`96.09% <100.00%> (+0.12%)`	⬆️

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 def9a6f...468e4f7. Read the comment docs.

travisbrown

Looks good to me. I'm happy to resolve the merge conflict (it's just imports following a testing change) if you'd like.

# Conflicts: # tests/src/test/scala/cats/tests/OptionTSuite.scala

travisbrown

👍 on green.

ybasket · 2020-03-12T12:03:06Z

@travisbrown thank you (also for the offer to resolve the conflict)! Just fixed the formatting, all checks green now.

travisbrown

Great, thanks, now we just need a second review!

diesalbla · 2020-04-11T13:36:04Z

+   * Perform an effect if the value inside the is a `None`, leaving the value untouched. Equivalent to [[orElseF]]
+   * with an effect returning `None` as argument.
+   */
+  def flatTapNone[B](f: => F[B])(implicit F: Monad[F]): OptionT[F, A] =


Suggested change

def flatTapNone[B](f: => F[B])(implicit F: Monad[F]): OptionT[F, A] =

def flatTapNone[B](ifNone: => F[B])(implicit F: Monad[F]): OptionT[F, A] =

diesalbla · 2020-04-11T13:36:29Z

+   * res0: List[Int] = List(23, 46)
+   * }}}
+   */
+  def foldF[B](default: => F[B])(f: A => F[B])(implicit F: FlatMap[F]): F[B] =


Suggested change

def foldF[B](default: => F[B])(f: A => F[B])(implicit F: FlatMap[F]): F[B] =

def foldF[B](ifNone: => F[B])(withSome: A => F[B])(implicit F: FlatMap[F]): F[B] =

Should we add more informative names?

While I agree with your intention (and have hence changed it on flatTapNone – thx), I would value consistency with fold more and keep the parameter names the same.

diesalbla · 2020-04-11T13:39:31Z

+   * one parameter list, which can result in better type inference in some
+   * contexts.
+   */
+  def cataF[B](default: => F[B], f: A => F[B])(implicit F: FlatMap[F]): F[B] =


I am aware of the notions of catamorphisms, but is that also a unified term or notion across cats? I can only grep the words def cata in OptionTand in CoFree.

Unless we want to extend the use across the whole of cats, perhaps we should drop them, to avoid confusion.

There's a cata to fold already, so when seeing foldF, I'd expect cataF there as well. Whether cata should have been introduced with that name in the first place seems to be outside of the scope of this PR…but that's just my two cents.

diesalbla · 2020-04-11T13:40:54Z

  def semiflatMap[B](f: A => F[B])(implicit F: Monad[F]): OptionT[F, B] =
    flatMap(a => OptionT.liftF(f(a)))

+  def semiflatTap[B](f: A => F[B])(implicit F: Monad[F]): OptionT[F, A] =


We have had this conversation before: given that the B has no relevance on the types of the outputs, should we just make it an existential?

Suggested change

def semiflatTap[B](f: A => F[B])(implicit F: Monad[F]): OptionT[F, A] =

def semiflatTap(withSome: A => F[_])(implicit F: Monad[F]): OptionT[F, A] =

FlatMap has this type parameter for flatTap, EitherT has it for it's semiFlatTap, so again, I would vote for consistency to give cats users a reliable experience in terms of how signatures look and when type inference works. Or do you see any other advantage of the existential which would outweigh this aspect?

…tiont # Conflicts: # tests/src/test/scala/cats/tests/OptionTSuite.scala

ybasket · 2020-05-13T14:41:13Z

@travisbrown As you approved the code once already and I changed only an argument name since then, would you mind merging? 🙏

travisbrown · 2020-05-13T15:26:28Z

Sure, I’ll merge when I’m back at a computer.

codecov-commenter · 2024-12-30T17:43:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.71%. Comparing base (def9a6f) to head (468e4f7).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3335   +/-   ##
=======================================
  Coverage   92.70%   92.71%           
=======================================
  Files         379      379           
  Lines        7981     7985    +4     
  Branches      230      230           
=======================================
+ Hits         7399     7403    +4     
  Misses        582      582

Yannick Heiber added 2 commits February 29, 2020 16:31

Add foldF, cataF and emptyflatMap to OptionT

12551ca

Make emptyflatTap more tolerant on type

a48ed5a

ybasket commented Feb 29, 2020

View reviewed changes

Comment thread core/src/main/scala/cats/data/OptionT.scala Outdated

Rename emptyflatTap to flatTapNone, fix docs

69c0f01

ybasket requested a review from rossabaker February 29, 2020 18:31

Add tests for new OptionT methods

3dfd6bb

ybasket force-pushed the more-utils-for-optiont branch from 7b1bfed to 3dfd6bb Compare February 29, 2020 23:21

Add OptionT.semiflatTap

6fa9007

travisbrown self-requested a review March 12, 2020 07:54

travisbrown previously approved these changes Mar 12, 2020

View reviewed changes

Merge branch 'master' into more-utils-for-optiont

ff699db

# Conflicts: # tests/src/test/scala/cats/tests/OptionTSuite.scala

ybasket dismissed travisbrown’s stale review via ff699db March 12, 2020 11:31

travisbrown self-requested a review March 12, 2020 11:32

travisbrown previously approved these changes Mar 12, 2020

View reviewed changes

fix formatting in OptionTSuite

f5d0419

ybasket dismissed travisbrown’s stale review via f5d0419 March 12, 2020 11:41

travisbrown self-requested a review March 12, 2020 12:34

travisbrown previously approved these changes Mar 12, 2020

View reviewed changes

travisbrown added this to the 2.2.0-M1 milestone Mar 20, 2020

diesalbla reviewed Apr 11, 2020

View reviewed changes

Yannick Heiber added 2 commits April 11, 2020 18:16

Merge remote-tracking branch 'upstream/master' into more-utils-for-op…

e3f83ab

…tiont # Conflicts: # tests/src/test/scala/cats/tests/OptionTSuite.scala

Rename OptionT.flatTapNone's argument for clarity

468e4f7

ybasket dismissed travisbrown’s stale review via 468e4f7 April 11, 2020 16:18

LukaJCB approved these changes Apr 14, 2020

View reviewed changes

LukaJCB merged commit cac6fdc into typelevel:master May 13, 2020

travisbrown modified the milestones: 2.2.0-M1, 2.2.0-M2 May 24, 2020

travisbrown added the enhancement label May 24, 2020

	def flatTapNone[B](f: => F[B])(implicit F: Monad[F]): OptionT[F, A] =
	def flatTapNone[B](ifNone: => F[B])(implicit F: Monad[F]): OptionT[F, A] =

	def foldF[B](default: => F[B])(f: A => F[B])(implicit F: FlatMap[F]): F[B] =
	def foldF[B](ifNone: => F[B])(withSome: A => F[B])(implicit F: FlatMap[F]): F[B] =

	def semiflatTap[B](f: A => F[B])(implicit F: Monad[F]): OptionT[F, A] =
	def semiflatTap(withSome: A => F[_])(implicit F: Monad[F]): OptionT[F, A] =

Uh oh!

Conversation

ybasket commented Feb 29, 2020

Uh oh!

Uh oh!

codecov-io commented Feb 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

travisbrown left a comment

Choose a reason for hiding this comment

Uh oh!

travisbrown left a comment

Choose a reason for hiding this comment

Uh oh!

ybasket commented Mar 12, 2020

Uh oh!

travisbrown left a comment

Choose a reason for hiding this comment

Uh oh!

diesalbla Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

diesalbla Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ybasket Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

diesalbla Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

ybasket Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

diesalbla Apr 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ybasket Apr 11, 2020

Choose a reason for hiding this comment

Uh oh!

ybasket commented May 13, 2020

Uh oh!

travisbrown commented May 13, 2020

Uh oh!

codecov-commenter commented Dec 30, 2024

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Feb 29, 2020 •

edited

Loading

diesalbla Apr 11, 2020 •

edited

Loading

diesalbla Apr 11, 2020 •

edited

Loading