Add Kleisli#mapK equivalent returning FunctionK #2450

kubukoz · 2018-08-31T22:11:22Z

I don't like the name either, so feel free to suggest a better one 😅

johnynek

thanks for the PR. I'm not crazy about mapKK as a name. Also, I'd like to exercise all code in a test. basically that this behaves the same as calling mapK on the instance should a good test.

kubukoz · 2018-08-31T22:18:25Z

Sure, I wanted to get some initial feedback before taking the time to write (a) test(s) :) as mentioned in the issue, I'd gladly rename it too, but mapK is taken (even though it's somewhere else, it clearly has different types) and I'm not as good at naming

kailuowang · 2018-09-12T17:06:43Z

This mapK type of operation keeps coming back, maybe we should reconsider this PR #1815. The liftNT (or we could name it liftNaturalTransformation ) provides this mapKK feature for all those data types through type class.

kubukoz · 2018-10-05T16:10:33Z

cats-tagless's FunctorK would be a generalization of mapK, but it has
def mapK[F[_], G[_]](af: A[F])(fk: F ~> G): A[G]
and doesn't provide anything of the shape A[F] ~> A[G].

I would start by defining this for Kleisli for now, and if we see this used by people enough to make an abstraction, maybe then we'll add a typeclass? (FunctorK)

kailuowang · 2018-10-05T17:44:50Z

I am ok with adding it to Kleisli first under the name liftK or liftNT

kubukoz · 2018-10-05T21:16:54Z

There's already a liftK. liftNT is free, but I'm afraid the lift prefix doesn't match what that function would do. After all, this operates on an already existing Kleisli, and the lifting functions create Kleislis out of values like F[B] or B. Here's a potentially stupid idea: fmapK, just to disambiguate it from mapK?

kailuowang · 2018-10-06T00:17:54Z

The lift method in Functor is the inspiration of this name.

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

I think liftNT or liftFunctionK is fine since it's about lifting a FunctionK to another FunctionK to the Kleisli space. lift is an general action, to be honest, I don't see much confusion if you have lift that lifts a F[B] or B to a Kleisli, and liftNT that lifts a FunctionK of F, G to another FunctionK of Kleisli[F] and Kleisli[G]

kubukoz · 2018-10-06T00:32:07Z

That makes sense, yeah. I think `liftNT` will be good enough then :)

…

On Sat, Oct 6, 2018, 02:18 Kai(luo) Wang ***@***.***> wrote: The lift method in Functor is the inspiration of this name. def lift[A, B](f: A => B): F[A] => F[B] = fa => map(fa)(f) I think liftNT or liftFunctionK is fine since it's about lifting a FunctionK to another FunctionK to the Kleisli space. lift is an general action, to be honest, I don't see much confusion if you have lift that lifts a F[B] or B to a Kleisli, and liftNT that lifts a FunctionK of F, G to another FunctionK of Kleisli[F] and Kleisli[G] — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2450 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2npAOZS0tFhXQqvdXk--qLg59UmQY0ks5uh_a9gaJpZM4WV6lg> .

kubukoz · 2018-10-07T14:19:23Z

Renamed to liftNT, added a test for consistency with mapK and some scaladoc - let's proceed :)

kubukoz · 2018-10-08T08:42:27Z

Apparently the addition isn't binary compatible, but only on 2.11. Any ideas, @kailuowang?

LukaJCB · 2018-10-08T08:44:57Z

Make a new trait KleisliFunctionsBinCompat, add the function there and then mix it into object Kleisli :)

codecov-io · 2018-10-08T09:26:54Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2450      +/-   ##
==========================================
+ Coverage   95.12%   95.12%   +<.01%     
==========================================
  Files         363      363              
  Lines        6704     6705       +1     
  Branches      289      284       -5     
==========================================
+ Hits         6377     6378       +1     
  Misses        327      327

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/Kleisli.scala	`97.27% <100%> (+0.02%)`	⬆️

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 b781a1a...55404d9. Read the comment docs.

kubukoz · 2018-10-08T09:27:56Z

hmm, now I see that you mentioned object Kleisli, won't it work if I mix it into KleisliFunctions?

Edit: no, it won't

kubukoz · 2018-11-07T15:55:49Z

@LukaJCB hi, is there anything else I need to do with this one?

LukaJCB · 2018-11-07T16:09:43Z

core/src/main/scala/cats/data/Kleisli.scala

@@ -111,21 +110,21 @@ object Kleisli extends KleisliInstances with KleisliFunctions with KleisliExplic
    }
 }

-sealed private[data] trait KleisliFunctions {
+private[data] sealed trait KleisliFunctions {


Was this done for a particular reason? :)

no, just a rebase issue - I'll revert

LukaJCB · 2018-11-07T16:10:08Z

core/src/main/scala/cats/data/Kleisli.scala

+    * scala> b.value.run("").value
+    * res0: Option[Int] = Some(1)
+    * }}}
+    */


This is IntelliJ autoformating, I assume?

ugh. For some reason it didn't enable scalafmt in cats, it seems...

LukaJCB · 2018-11-07T16:10:53Z

Sorry @kubukoz, it looks good, just a couple of nitpicks :)

kailuowang · 2018-11-09T14:11:18Z

@kubukoz build failed due to scalafmt check, would you run sbt prePR and commit the changes?

kubukoz · 2018-11-09T21:30:08Z

Done :)

kubukoz · 2018-12-05T18:55:23Z

@kailuowang WDYT about merging this?

kailuowang · 2018-12-05T19:01:58Z

@kubukoz sorry I dropped the ball on this one. And after another look, I am not sure about the liftNT name I suggested. We renamed NaturalTransformation to FunctionK a while ago so NT might be confusing. Would it be possible to rename it to liftFK or liftFunctionK? Sorry about the back and forth.

kubukoz · 2018-12-05T19:06:02Z

It's okay, I know how important it is to get the API right when you can't rename it for a while :)

We can definitely rename, and although I like shorter names I'll say liftFK's FK part reminds me too much of "foreign key" and I'd go with liftFunctionK to be more explicit.

kailuowang · 2018-12-05T19:09:34Z

Thanks! liftFunctionK sounds good to me.

Fix typo Remove whitespace Rename mapKK to liftNT, add test, add scaladoc Fix doctests Move `liftNT` to bincompat trait Extend bincompat trait in `Kleisli`'s companion object Remove rebase remainder

LukaJCB · 2018-12-07T10:58:05Z

I think you renamed the wrong function :P

kubukoz · 2018-12-07T10:59:00Z

Yeah, just noticed. Time does things to people...

kubukoz · 2018-12-07T17:13:02Z

(Rerunning due to the neverending Travis build on 2.13)

kubukoz mentioned this pull request Aug 31, 2018

Add ContextShift#evalOnK typelevel/cats-effect#334

Merged

johnynek reviewed Aug 31, 2018

View reviewed changes

LukaJCB reviewed Nov 7, 2018

View reviewed changes

LukaJCB previously approved these changes Nov 7, 2018

View reviewed changes

kailuowang previously approved these changes Nov 7, 2018

View reviewed changes

kubukoz dismissed stale reviews from kailuowang and LukaJCB via 34f3688 November 9, 2018 21:21

kubukoz added 2 commits December 7, 2018 11:52

Add Kleisli#mapK equivalent returning FunctionK

c82d087

Fix typo Remove whitespace Rename mapKK to liftNT, add test, add scaladoc Fix doctests Move `liftNT` to bincompat trait Extend bincompat trait in `Kleisli`'s companion object Remove rebase remainder

Restore modifier order & formatting of Kleisli.scala

0f5f79b

kubukoz added 3 commits December 7, 2018 11:52

Formatting

22177e0

Rename to liftFunctionK

3cb5b2e

oops, wrong function

4f6c2e2

LukaJCB approved these changes Dec 7, 2018

View reviewed changes

dummy commit to force travis

55404d9

kailuowang approved these changes Dec 7, 2018

View reviewed changes

kailuowang merged commit 4d98948 into typelevel:master Dec 7, 2018

kailuowang added this to the 1.6 milestone Jan 8, 2019

kailuowang added the enhancement label Jan 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Kleisli#mapK equivalent returning FunctionK #2450

Add Kleisli#mapK equivalent returning FunctionK #2450

kubukoz commented Aug 31, 2018 •

edited

Loading

johnynek left a comment

kubukoz commented Aug 31, 2018 •

edited

Loading

kailuowang commented Sep 12, 2018

kubukoz commented Oct 5, 2018

kailuowang commented Oct 5, 2018

kubukoz commented Oct 5, 2018

kailuowang commented Oct 6, 2018 •

edited

Loading

kubukoz commented Oct 6, 2018 via email

kubukoz commented Oct 7, 2018

kubukoz commented Oct 8, 2018

LukaJCB commented Oct 8, 2018 •

edited

Loading

codecov-io commented Oct 8, 2018 •

edited

Loading

kubukoz commented Oct 8, 2018 •

edited

Loading

kubukoz commented Nov 7, 2018

LukaJCB Nov 7, 2018

kubukoz Nov 7, 2018

LukaJCB Nov 7, 2018

kubukoz Nov 7, 2018

LukaJCB commented Nov 7, 2018

kailuowang commented Nov 9, 2018

kubukoz commented Nov 9, 2018

kubukoz commented Dec 5, 2018

kailuowang commented Dec 5, 2018

kubukoz commented Dec 5, 2018

kailuowang commented Dec 5, 2018

LukaJCB commented Dec 7, 2018

kubukoz commented Dec 7, 2018

kubukoz commented Dec 7, 2018

Add Kleisli#mapK equivalent returning FunctionK #2450

Add Kleisli#mapK equivalent returning FunctionK #2450

Conversation

kubukoz commented Aug 31, 2018 • edited Loading

johnynek left a comment

Choose a reason for hiding this comment

kubukoz commented Aug 31, 2018 • edited Loading

kailuowang commented Sep 12, 2018

kubukoz commented Oct 5, 2018

kailuowang commented Oct 5, 2018

kubukoz commented Oct 5, 2018

kailuowang commented Oct 6, 2018 • edited Loading

kubukoz commented Oct 6, 2018 via email

kubukoz commented Oct 7, 2018

kubukoz commented Oct 8, 2018

LukaJCB commented Oct 8, 2018 • edited Loading

codecov-io commented Oct 8, 2018 • edited Loading

Codecov Report

kubukoz commented Oct 8, 2018 • edited Loading

kubukoz commented Nov 7, 2018

LukaJCB Nov 7, 2018

Choose a reason for hiding this comment

kubukoz Nov 7, 2018

Choose a reason for hiding this comment

LukaJCB Nov 7, 2018

Choose a reason for hiding this comment

kubukoz Nov 7, 2018

Choose a reason for hiding this comment

LukaJCB commented Nov 7, 2018

kailuowang commented Nov 9, 2018

kubukoz commented Nov 9, 2018

kubukoz commented Dec 5, 2018

kailuowang commented Dec 5, 2018

kubukoz commented Dec 5, 2018

kailuowang commented Dec 5, 2018

LukaJCB commented Dec 7, 2018

kubukoz commented Dec 7, 2018

kubukoz commented Dec 7, 2018

kubukoz commented Aug 31, 2018 •

edited

Loading

kubukoz commented Aug 31, 2018 •

edited

Loading

kailuowang commented Oct 6, 2018 •

edited

Loading

LukaJCB commented Oct 8, 2018 •

edited

Loading

codecov-io commented Oct 8, 2018 •

edited

Loading

kubukoz commented Oct 8, 2018 •

edited

Loading