Add mapK to transformers #1987

andyscott · 2017-10-21T22:28:24Z

Adds mapK to the following:

cats.data.EitherK
cats.data.EitherT
cats.data.Func
cats.data.IdT
cats.data.IndexedReaderWriterStateT
cats.data.IndexedStateT
cats.data.Kleisli
cats.data.Nested
cats.data.OneAnd
cats.data.OptionT
cats.data.Tuple2K
cats.data.WriterT
cats.free.Coyoneda
cats.free.Free
cats.free.FreeT
cats.free.Yoneda

The following are marked deprecated in favor of mapK:

cats.data.Kleisli#transform
cats.free.Coyoneda#transform
cats.free.Free#compile
cats.free.Free.compile
cats.free.FreeT#hoist

The primary motivation for this is to allow easy (and consistent) access to the transformer's underlying effect. For example, given a computation OptionT[Task, Int], you may want to call some of the underlying methods provided by Task.

LukaJCB · 2017-10-21T22:30:01Z

This is great! Thanks for this. :) You do have some conflicts though. I also left a couple of tiny comments.

LukaJCB · 2017-10-21T22:30:44Z

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

@@ -234,4 +234,3 @@ private[data] trait EitherKComonad[F[_], G[_]] extends Comonad[EitherK[F, G, ?]]
  def extract[A](p: EitherK[F, G, A]): A =
    p.extract
 }
-


This is by accident?

That's Emacs doing some extra housekeeping from having the file open. I'll revert it.

Also-- you're fast! This PR was up for just a few minutes and you were already on top of it 😄.

Haha, yeah, sorry for nitpicking so quickly 😄

LukaJCB · 2017-10-21T22:31:17Z

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

@@ -18,8 +18,11 @@ final case class Kleisli[F[_], A, B](run: A => F[B]) { self =>
  def map[C](f: B => C)(implicit F: Functor[F]): Kleisli[F, A, C] =
    Kleisli(a => F.map(run(a))(f))

-  def mapF[N[_], C](f: F[B] => N[C]): Kleisli[N, A, C] =
-    Kleisli(run andThen f)


Maybe we can keep this as a deprecated alias? :)

I didn't mean to remove it-- I had added mapF to every transformer but wound up splitting off mapK additions to a separate PR. This just an accidental carry over from the split!

LukaJCB · 2017-10-21T22:42:57Z

Oh and can you also add it to FreeT? :)

andyscott · 2017-10-21T22:47:40Z

FreeT already has mapK a la hoist. I'll add mapK and deprecate hoist. Additionally, I'll add mapK to the yoneda duals.

codecov-io · 2017-10-21T23:10:16Z

Codecov Report

Merging #1987 into master will decrease coverage by 0.09%.
The diff coverage is 70.58%.

@@            Coverage Diff            @@
##           master    #1987     +/-   ##
=========================================
- Coverage   96.21%   96.12%   -0.1%     
=========================================
  Files         272      272             
  Lines        4627     4640     +13     
  Branches      115      117      +2     
=========================================
+ Hits         4452     4460      +8     
- Misses        175      180      +5

Impacted Files	Coverage Δ
free/src/main/scala/cats/free/Yoneda.scala	`80% <0%> (-20%)`	⬇️
free/src/main/scala/cats/free/Coyoneda.scala	`84.61% <0%> (-7.06%)`	⬇️
core/src/main/scala/cats/data/WriterT.scala	`94.25% <100%> (+0.06%)`	⬆️
core/src/main/scala/cats/data/EitherT.scala	`97.58% <100%> (+0.01%)`	⬆️
core/src/main/scala/cats/data/IdT.scala	`97.36% <100%> (+0.07%)`	⬆️
...in/scala/cats/data/IndexedReaderWriterStateT.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/OptionT.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/IndexedStateT.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/Kleisli.scala	`98.64% <50%> (-1.36%)`	⬇️
free/src/main/scala/cats/free/FreeT.scala	`90.41% <75%> (-1.26%)`	⬇️

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 7b4814a...d623055. Read the comment docs.

andyscott · 2017-10-21T23:11:01Z

I've marked a few items as deprecated. If we'd like to keep the deprecation annotations, I'll need to know the release this PR is scheduled for and update them accordingly.

johnynek · 2017-10-22T02:36:42Z

Can we not have a typeclass here? Seems like we want the analog to Functor no? Any reason not to add with so many instances? The composition law is pretty obvious. Seems worth it to me.

andyscott · 2017-10-22T07:00:05Z

Definitely. There has been related discussion in the past, starting with #1713.

I'd like to add a FunctorK in the same manner as @kailuowang's mainecoon: https://github.com/kailuowang/mainecoon/blob/master/core/src/main/scala/mainecoon/FunctorK.scala.

johnynek · 2017-10-22T12:29:52Z

If we are adding mapK I think we should only do it if we also add that FunctorK. I don’t think this should be controversial. Look at how many instances there are.

On Sat, Oct 21, 2017 at 21:00 andy scott ***@***.***> wrote: Definitely. There has been related discussion in the past, starting with #1713 <#1713>. I'd like to add a FunctorK in the same manner as @kailuowang <https://github.com/kailuowang>'s mainecoon: https://github.com/kailuowang/mainecoon/blob/master/core/src/main/scala/mainecoon/FunctorK.scala . — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1987 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdt_YM0sTNPJ7Sxqy4QxkTW8Egcmaks5suuf2gaJpZM4QBvO_> .

-- P. Oscar Boykin, Ph.D. | http://twitter.com/posco | http://pobox.com/~boykin

andyscott · 2017-10-22T19:06:38Z

https://github.com/andyscott/cats/pull/1/files has FunctorK and instances for OptionT and EitherT. If that looks good, I'll give the same treatment to the rest of the instances.

A few notes on my implementation:

For a given transformer, FunctorK is invariant with regards to all non-effect parameters and requires no additional evidence. At the value level, only one instance is needed-- so I use a cast to specialize for non-effect parameters. This can be switched to the traditional implementation if desired.

There's no way easy way to provide arbitrary FunctionK transformations, so the laws for FunctorK just ask for specific transformations to test. In Mainecoon, laws ask for arbitrary instances but elsewhere Gen.const based instances are provided. If we have to use constant instances, I would prefer to pass them in as parameters for clarity.

kailuowang · 2017-10-23T15:46:07Z

There are two contention points IRT the type class,

what does such an abstraction buy us? In another word, is there a realworld scenario where someone would need a def something[A[_[_]]: FunctorK] ?
should the type class be of A[_[_]] or A[_[_], _]? A[_[_]] does cover more types, but due to the limitation of Scala syntax, it has a shortcoming, that is you cannot fix the effect type F[_] inside of it and get a type constructor. A[F] will have to be a concrete type, it cannot be a type constructor. On the other hand, A[_[_], _] can still be a type constructor even if you fix the effect type, since you can write A[F, _]. This gives you the ability to lift a F[_] ~> G[_] to A[F, _] ~> A[G, _], which makes it more like a higher kinded endofunctor with a fmap on FunctionK, and possibly enable more interesting operations like possibly composeK, etc.

On the other hand, both definitions of this type classes ( A[_[_]] or A[_[_], _]) probably don't matter that much because, again, there is no real use case for the abstraction anyway, the only use case is for people to call mapK on it.
To me the main incentive of adding such a type class would be

having a unified law testing code and
some possibly beneficial utility functions in the type class such as liftK: F[_] ~> G[_] => A[F, _] ~> A[G, _] and composeK: (FunctorK[A[_[_], _]], FunctorK[B[_[_], _]]) => FunctorK[λ[(a[_], b) => A[B[a, _], b]]].

@andyscott what's your incentive for this to be in the same manner as in mainecoon? to support final tagless encoding or do you have some other use cases?

Update: In mainecoon, there is a use case for the FunctorK[A[_[_]] abstraction. I have auto derivation code like this
implicit def derive[A[_[_]]: FunctorK, F[_], G[_])(implicit af: A[F], fk: F ~> G): A[G] = af.mapK(fk)
With this given an algebra with FunctorK and an available interpreter, I can automatically derive an interpreter of any effect type as long as there is a FunctionK route from the available interpreter to this effect type.
This sort of auto derivation can only be achieved with a type class.

LukaJCB · 2017-10-23T16:25:04Z

I agree with @kailuowang that it's not very useful in general as a typeclass atleast for cats-core. The only reason I could see, is that it might be useful for both cats-mtl and mainecoon which both depend on cats-core, so they don't have to define their own separately.

andyscott · 2017-10-23T17:10:54Z

I originally just needed something similar to mapK to modify my transformer stack with `F ~> F` transformations. Specifically I wanted an easier way to call methods directly on Monix Tasks. This PR is the result of various discussions related to that. On Oct 23, 2017 09:25, "LukaJCB" <notifications@github.com> wrote: I agree with @kailuowang <https://github.com/kailuowang> that it's not very useful in general as a typeclass atleast for cats-core. The only reason I could see, is that it might be useful for both cats-mtl and mainecoon which both depend on cats-core, so they don't have to define their own separately. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1987 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAS8WxIhD2gprzkadqeqT-Dneq4U1VMrks5svL3mgaJpZM4QBvO_> .

iravid · 2017-10-23T17:19:22Z

Quick reminder - StateT won't have an instance; it imposes a F: Functor constraint due to the F[S => F[(S, A)]] encoding.

kailuowang · 2017-10-23T17:31:57Z

@LukaJCB the need for a FunctorK in cats-mtl and mainecoon is actually different. In cats-mtl it took the shape of FunctorK[A[_[_], _]] until edmund decided it's not worth the abstraction, in mainecoon, it is required to have the shape of FunctorK[A[_[_]]], because that's the shape of final tagless encoded algebras.

LukaJCB · 2017-10-23T17:57:21Z

Ah you're right! In that case, my vote goes to merging it as is right now :)
I can definitely empathize with @andyscott's situation, as I have wanted the exact same thing in the past 😄

andyscott · 2017-10-23T18:10:26Z

@iravid Yep. Everything except IRWST and IndexedStateT could have an instance if we add FunctorK.

andyscott · 2017-10-24T16:39:02Z

I think all feedback has been addressed. Codecov has failed the PR because the deprecated methods don't have coverage (they simply forward to mapK).

kailuowang · 2017-10-24T17:16:58Z

@andyscott it can also be added to

EitherK (on the right F)
Tuple2K (on the right F)
Free
Func
Nested
OneAnd
Also I noticed that tests/src/test/scala/cats/tests/IndexedReaderWriterStateTTests.scala
should've been renamed to tests/src/test/scala/cats/tests/IndexedReaderWriterStateTSuite.scala
Let me know if you have time to add those , if not I'll be more than happy to submit a PR against your branch.

@johnynek shall we unify the methods first in this PR and determine the type class later. There have been several attempts to add this FunctorK type class, none of them reached the consensus. If anything that should tell us that we shouldn't rush it into cats.core 1.0.
On the other hand, these straightforward mapK methods have been needed and I don't see any harm of having them without the type class.

andyscott · 2017-10-24T19:57:41Z

@kailuowang I'll take care of them within the next day.

johnynek · 2017-10-24T20:05:22Z

I guess we can just copy this pattern all over.

I kind of hate that in a library about typeclasses we are ignoring this.

Also, I think it would compose in a nice way for the transformers. So, if you have OptionT[EitherT[F, A, B]] you could compose to point into something deeper.

Of course, the scala type system issues are a pain, so I guess if we don't have a clean way to write this typeclass we should punt.

andyscott · 2017-10-24T20:08:55Z

There have been discussions on having a cats-more or extras module. While FunctorK doesn't provide a ton of utility to end users of cats, it would provide utility to other library authors (and anyone who wants to check the laws). Maybe the typeclass should go there?

see #1940

johnynek · 2017-10-24T20:16:01Z

I'm actually -1 on tiny modules that we then have to bolt on a ton of boiler plate and imports.

FunctorK if we did it right would be a trivial increase in the size of cats. Having to add an additional import, build setup, implicit imports, etc... seems unmotivated to me.

The main motivations, in my view, for splitting modules are:

decrease jar size when many don't need everything. This should be done when we can save megabytes, not kilobytes.
reduce dependencies, so users who do not need a module don't complicate their build problem with additional dependencies.

I really don't think there are significant reasons to split other than that. This does not seem to be one of those two cases.

andyscott · 2017-10-24T20:22:49Z

I'm also -1 on tiny modules for the same reasons. But if we can't add something to the core module and it's valuable, then having it in a tiny module is arguably better than not having it at all.

LukaJCB · 2017-10-24T20:26:53Z

We should continue the discussion here #1713. And like @kailuowang said we could potentially still add it if we come to a good conclusion.

andyscott · 2017-10-29T19:06:50Z

I've added missing instances. The remaining tests still need to be added.

When adding the method to Free, I realized I have it set up so mapK on Free operates on the free functor S but mapK on FreeT operates on the transformer context M. Should mapK for FreeT also operate on the functor S?

kailuowang · 2017-10-30T18:03:52Z

free/src/main/scala/cats/free/Free.scala

-      λ[FunctionK[S, Free[T, ?]]](fa => Suspend(f(fa)))
-    }(Free.catsFreeMonadForFree)
+    */
+  @deprecated("Use mapK", "1.0.0")


I am not sure if we want to deprecate this one. compile seems a meaningful alias.

kailuowang · 2017-10-30T18:42:01Z

I am fine withmapK operates on S on Free and M on FreeT .

johnynek

looks good. One minor comment.

johnynek · 2017-10-31T00:25:12Z

free/src/main/scala/cats/free/Free.scala

@@ -218,10 +231,17 @@ object Free extends FreeInstances {
    pure(()).flatMap(_ => value)

  /**
+   * a FunctionK, suitable for composition, which calls mapK
+   */
+  def mapK[F[_], G[_]](fk: FunctionK[F, G]): FunctionK[Free[F, ?], Free[G, ?]] =


why FunctionK[F, G] here but F ~> G in most other places?

This method is a copy-paste-rename from the original Free.compile. I'll go through and switch FunctionK to ~> in the files modified by this PR, for consistency.

LukaJCB · 2017-10-31T10:36:20Z

The Travis build fails because the Parallel instance for Kleisli uses the transform method which is now deprecated, you might need to rebase and change that one :)
Otherwise LGTM! 🎉

kailuowang · 2017-10-31T12:11:17Z

Actually can we also not deprecate hoist that's a common alias.

kailuowang · 2017-10-31T14:08:15Z

@andyscott we continued this in #2000 so that we can release this morning. Sorry for the rush. thanks for this awesome contribution.

kailuowang · 2017-10-31T15:05:13Z

closing as #2000 is merged

LukaJCB reviewed Oct 21, 2017

View reviewed changes

andyscott force-pushed the feature/add-mapK branch from 52cb546 to 208fbd0 Compare October 21, 2017 22:33

Add mapK to most (hopefully all) transformers

5fca942

andyscott force-pushed the feature/add-mapK branch from 208fbd0 to 5fca942 Compare October 21, 2017 22:36

Add mapK to free related constructs

d0d061f

Deprecate Kleisli#transform in favor of mapK

2c4e753

andyscott changed the title ~~Add mapK to most (hopefully all) transformers~~ Add mapK to transformers Oct 23, 2017

kailuowang added this to the 1.0.0 milestone Oct 27, 2017

andyscott added 2 commits October 29, 2017 11:27

Fix tests file name

99a1510

Add missing mapK methods

d623055

kailuowang reviewed Oct 30, 2017

View reviewed changes

kailuowang added the Source Breaking label Oct 30, 2017

johnynek reviewed Oct 31, 2017

View reviewed changes

LukaJCB mentioned this pull request Oct 31, 2017

Finish adding mapK #2000

Merged

kailuowang closed this Oct 31, 2017

kailuowang modified the milestones: 1.0.0, 1.0.0-RC1 Oct 31, 2017

Add mapK to transformers #1987

Add mapK to transformers #1987

Conversation

andyscott commented Oct 21, 2017 • edited Loading

LukaJCB commented Oct 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Oct 21, 2017

andyscott commented Oct 21, 2017 • edited Loading

codecov-io commented Oct 21, 2017 • edited Loading

Codecov Report

andyscott commented Oct 21, 2017

johnynek commented Oct 22, 2017

andyscott commented Oct 22, 2017

johnynek commented Oct 22, 2017 via email

andyscott commented Oct 22, 2017 • edited Loading

kailuowang commented Oct 23, 2017 • edited Loading

LukaJCB commented Oct 23, 2017

andyscott commented Oct 23, 2017 via email

iravid commented Oct 23, 2017

kailuowang commented Oct 23, 2017

LukaJCB commented Oct 23, 2017

andyscott commented Oct 23, 2017 • edited Loading

andyscott commented Oct 24, 2017

kailuowang commented Oct 24, 2017

andyscott commented Oct 24, 2017

johnynek commented Oct 24, 2017

andyscott commented Oct 24, 2017 • edited Loading

johnynek commented Oct 24, 2017

andyscott commented Oct 24, 2017

LukaJCB commented Oct 24, 2017

andyscott commented Oct 29, 2017 • edited Loading

Choose a reason for hiding this comment

kailuowang commented Oct 30, 2017 • edited Loading

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Oct 31, 2017

kailuowang commented Oct 31, 2017

kailuowang commented Oct 31, 2017

kailuowang commented Oct 31, 2017

andyscott commented Oct 21, 2017 •

edited

Loading

LukaJCB commented Oct 21, 2017 •

edited

Loading

andyscott commented Oct 21, 2017 •

edited

Loading

codecov-io commented Oct 21, 2017 •

edited

Loading

andyscott commented Oct 22, 2017 •

edited

Loading

kailuowang commented Oct 23, 2017 •

edited

Loading

andyscott commented Oct 23, 2017 •

edited

Loading

andyscott commented Oct 24, 2017 •

edited

Loading

andyscott commented Oct 29, 2017 •

edited

Loading

kailuowang commented Oct 30, 2017 •

edited

Loading