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 tapWithF to Kleisli #2252

Merged
merged 1 commit into from
May 19, 2018
Merged

Conversation

marcodippy
Copy link
Contributor

No description provided.

@@ -75,6 +75,9 @@ final case class Kleisli[F[_], A, B](run: A => F[B]) { self =>
def tapWith[C](f: (A, B) => C)(implicit F: Functor[F]): Kleisli[F, A, C] =
Kleisli(a => F.map(run(a))(b => f(a, b)))

def flatTapWith[C](f: (A, B) => F[C])(implicit F: FlatMap[F]): Kleisli[F, A, C] =
Copy link
Contributor

Choose a reason for hiding this comment

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

we have this function flatTap on anything with a FlatMap, but it does not seem consistent with this:

def flatTap[A, B](fa: F[A])(f: A => F[B]): F[A] =

it discards the value inside the F[C].

I am concerned we don't have a clear meaning associated with tap and flatTap.

Copy link
Contributor Author

@marcodippy marcodippy May 9, 2018

Choose a reason for hiding this comment

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

I agree with you @johnynek, but I couldn't come up with a better name for this one (flatTapWith felt "consistent" with the tapWith function a couple of lines above)

I'm definitely open to suggestions on the name :)

Copy link
Contributor

Choose a reason for hiding this comment

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

tapWithF?

We could rewrite (A, B) => F[C] to A => (B => F[C]) if we had use F.pure on the result we could have done: A => F[B => F[C]]

Then this method could be written as ap followed by flattenF or something.

Just noodling around to think about naming.

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 like tapWithF, thanks for the suggestion @johnynek!
I already pushed the change 👍

@codecov-io
Copy link

codecov-io commented May 9, 2018

Codecov Report

Merging #2252 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2252      +/-   ##
==========================================
+ Coverage   94.96%   94.96%   +<.01%     
==========================================
  Files         333      333              
  Lines        5797     5798       +1     
  Branches      221      218       -3     
==========================================
+ Hits         5505     5506       +1     
  Misses        292      292
Impacted Files Coverage Δ
core/src/main/scala/cats/data/Kleisli.scala 96.93% <100%> (+0.03%) ⬆️

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 cb2a08b...045c101. Read the comment docs.

@marcodippy marcodippy changed the title Add flatTapWith to Kleisli Add tapWithF to Kleisli May 13, 2018
@johnynek johnynek merged commit 13de346 into typelevel:master May 19, 2018
@kailuowang kailuowang added this to the 1.2 milestone May 30, 2018
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.

6 participants