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

mapper for `F[_]` should be generic at kind level #1045

Merged
merged 15 commits into from Dec 7, 2018

Conversation

Projects
None yet
5 participants
@jcouyang
Copy link
Contributor

jcouyang commented Nov 29, 2018

just for demonstrating #1044

while it's generic all mapperFrom*Future* can be deleted

and user can implement F ~> E:Effect for their own F whenever/where ever they want

same thing should done to mapper for response as well

@vkostyukov

This comment has been minimized.

Copy link
Member

vkostyukov commented Nov 30, 2018

@sergeykolbasov @rpless Do you have an opinion?

@jcouyang Mind fixing a build in the meantime? I think scalafix wasn't happy about something - just check the build logs or verify it with sbt validate locally.

@sergeykolbasov

This comment has been minimized.

Copy link
Member

sergeykolbasov commented Nov 30, 2018

I'm still not sold to the FunctionK approach instead of custom type class due to the fact that those implicit instances are supposed to be used all around the place and not only in Mapper. With this approach we have to carry around import with implicits inside instead of letting compiler to find it in companion object on itself.

jcouyang added some commits Dec 1, 2018

@jcouyang jcouyang force-pushed the jcouyang:issue1044 branch from a50fe52 to 1c2ed72 Dec 1, 2018

@jcouyang

This comment has been minimized.

Copy link
Contributor Author

jcouyang commented Dec 1, 2018

again @sergeykolbasov i'm not saying not using typeclass

what i'm trying to say is definitely we need typeclass, just you don't need to define your own since cats have it already

imagine that your typeclass would probably looks like

trait ToEffect[F[_], E[_]:Effect] {
  def apply[A](a: F[A]): E[A]
}

just look at the type, it's exactly the same as FunctionK/~>

trait FunctionK[F[_], G[_]] {
  def apply[A](fa: F[A]): G[A]
}

either way the implementation of the typeclass for future or any user custom type should be exactly the same

jcouyang added some commits Dec 1, 2018

Revert "scala fix"
This reverts commit df2e2ae.
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Dec 1, 2018

Codecov Report

❗️ No coverage uploaded for pull request base (master@9f677b3). Click here to learn what that means.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1045   +/-   ##
=========================================
  Coverage          ?   83.88%           
=========================================
  Files             ?       51           
  Lines             ?      943           
  Branches          ?       63           
=========================================
  Hits              ?      791           
  Misses            ?      152           
  Partials          ?        0
Impacted Files Coverage Δ
core/src/main/scala/io/finch/package.scala 100% <ø> (ø)
core/src/main/scala/io/finch/internal/Mapper.scala 100% <100%> (ø)
.../src/main/scala/io/finch/internal/IdToEffect.scala 100% <100%> (ø)
.../main/scala/io/finch/internal/FutureToEffect.scala 87.5% <87.5%> (ø)

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 9f677b3...8a73877. Read the comment docs.

@@ -3,6 +3,8 @@ package io.finch
import cats.effect.IO
import com.twitter.finagle.http.Response
import com.twitter.util.{Future => TwitterFuture}
import io.finch.instances.scalaFuture._

This comment has been minimized.

@jcouyang

jcouyang Dec 1, 2018

Author Contributor

also moved scala and twitter future into instance package because they are just implementation of typeclass ?~>Effect, as cats convention
it still works if leave it in Mapper but i think it shouldn't belong to mapper anymore, mapper is generic for any kind now

This comment has been minimized.

@sergeykolbasov

sergeykolbasov Dec 1, 2018

Member

Well, that's exactly my point. We could avoid imports here and there using home-backed type class with companion object.
https://github.com/finagle/finch/blob/b03b88ad2339cebc1a106bb4f203d64895fde56e/core/src/main/scala/io/finch/internal/LiftToEffect.scala

This comment has been minimized.

@jcouyang

jcouyang Dec 1, 2018

Author Contributor

Still not getting your point. You can move where ever you want, they are implicits
I just think they should organized this way in cats philosophy
Imagine If I implement your type class LiftToEffect for my type in my code, those future will be auto import to my context which I don't need at all.
How is that better than explicitly import

This comment has been minimized.

@sergeykolbasov

sergeykolbasov Dec 1, 2018

Member

I just think they should organized this way in cats philosophy

Based on my experience working with different people through the years, this is one of the most frustrating pain points they had with Scala. It's not enough to write an actual code anymore but also requires a user to know what and when to import.

those future will be auto import to my context which I don't need at all.

They aren't auto-imported, but only by need. As far as you don't use these specific types in the context where type-class is involved, compiler doesn't care much about those implicit instances. This pattern is widely adopted in finch. Examples:

This comment has been minimized.

@jcouyang

jcouyang Dec 1, 2018

Author Contributor

It's not enough to write an actual code anymore but also requires a user to know what and when to import

I totally understand what your concern is and I think
cats already have pretty good pattern to cater both kind of users

for peeps don't know what to import, they simply import everything and let the compile pick for them

import cats.implicits._

for peeps know exactly what they need, i.e. functor instance of list

import cats.instances.list._

they both behave the same from user point of view but I believe the second one is more gentle to the compiler and more explicit about the intention

They aren't auto-imported, but only by need

they aren't at run time but are there for compiler search

This pattern is widely adopted in finch

i think decoupling typeclass and data type is the pattern that functional programming different from OO. it make sense to put all instance of data type in data type's companion object but not in typeclass's companion object
i.e.
when import cats.effect.IO you have all instances of datatype IO for typeclass Monad, Effect etc

but when you import cats.effect.Effect you should only have typeclass Effect, because when i import the typeclass, it means I want to implement it for my data type, which means I'm using custom data type, not IO, all instance of IO for that typeclass is the last thing I need.

since finch adopted cats, i reckon keeping only one pattern is easier for both maintainer and user

This comment has been minimized.

@sergeykolbasov

sergeykolbasov Dec 1, 2018

Member

I see your point here, but in my opinion, it's rather subjective and based on the experience with a certain environment. My argument comes from the UX point of view, rather than FP (or any other) purity. Extrapolating the example of Encode and ToResponse toward the state where every implicit is placed under a separate package, we end up either with dozens of imports for each primitive type like Int, Boolean or String, and/or introduce the single import of truth io.finch.implicits._, which is, sorry for being blunt, just a cargo cult.

This comment has been minimized.

@jcouyang

jcouyang Dec 2, 2018

Author Contributor

From UX point of view, I reckon user choose Finch over other is because fp since Finch branding itself so isn't it. At least I am because it's compatible with cats ecosystem and I expected seamless UX as cats.
If user come from other experience other than cats isn't finatra the a better choice then

This comment has been minimized.

@rpless

rpless Dec 2, 2018

Contributor

I'd like to respond as user, not a maintainer. The team I work with chose Finch 3 years ago because it was easy to use, very concurrent, and gave us all the benefits of Finagle to use as we needed. We've never followed how Cats does usability things (and my teammates have struggled with Cats' conventions), I that find while I understand the import strategy, I'm constantly teaching it or reminding others of it. While I'd personally be willing to accept another import (and this PR), we can't discount the users. I'm with @sergeykolbasov on this one.I think we should let it be discovered by the compiler.

@jcouyang jcouyang changed the title mapper for `F[Output]` should be generic at kind level mapper for `F[_]` should be generic at kind level Dec 1, 2018

@vkostyukov
Copy link
Member

vkostyukov left a comment

Thanks for following through, @jcouyang! I think I like the general idea - just a few nits from my side.

Show resolved Hide resolved core/src/main/scala/io/finch/package.scala Outdated
import scala.concurrent.{Future => ScalaFuture}
import scala.util.{Failure, Success}

trait ToIO {

This comment has been minimized.

@vkostyukov

vkostyukov Dec 3, 2018

Member

What's the reasoning for this being a trait? Why not move these two instances under ToEffect companion object?

This comment has been minimized.

@jcouyang

jcouyang Dec 4, 2018

Author Contributor

abstract them as trait so ToEffect could be more extensible, since IO is only one data type that implemented Effect[_], there could be other i.e. monix's Task, so that we could easily just implement Effect[Tasks] as trait ToTask and

object ToEffect extends instances.ToIO with instances.ToTask

This comment has been minimized.

@sergeykolbasov

sergeykolbasov Dec 4, 2018

Member

Would it be shorter to generalize it for any E : Effect instead?

This comment has been minimized.

@jcouyang

jcouyang Dec 4, 2018

Author Contributor

🤔 good idea, these Future things just instances of ToEffect[?, E]

@@ -1,13 +1,11 @@
package io.finch.internal

This comment has been minimized.

@vkostyukov

vkostyukov Dec 3, 2018

Member

I think normally we just leave the package name as a single fqn.

jcouyang added some commits Dec 4, 2018

Show resolved Hide resolved core/src/main/scala/io/finch/package.scala Outdated
import cats.effect.Effect
import io.finch.ToEffect

trait IdEffect {

This comment has been minimized.

@vkostyukov

vkostyukov Dec 4, 2018

Member

I lost our previous conversion on this, but I'm still not sure why we need these instances defined outside of ToEffect companion object?

This comment has been minimized.

@jcouyang

jcouyang Dec 4, 2018

Author Contributor

the rational is that comparing to put everything in one single big object, it's more extensible and modular by putting ToEffect's instances in different files
the implementation of ToEffect[E,E] here is totally irrelevant to the implementation of ToEffect[*Future, E](ideally TwtterFuture and ScalaFuture should be separated too but I think it's fine that they share some similarity)

This comment has been minimized.

@vkostyukov

vkostyukov Dec 4, 2018

Member

Extensible in a sense of for Finch users or for Finch developers? I'm trying to understand if there is any value in the latter.

This comment has been minimized.

@jcouyang

jcouyang Dec 5, 2018

Author Contributor

for finch developers, e.g. if finch want to natively support some new type other then Future, Id, it's easier to add new trait for object ToEffect to extends

user shouldn't care the details of these traits so that's why I move them back to internal folder

This comment has been minimized.

@vkostyukov

vkostyukov Dec 5, 2018

Member

Sorry, I'm a little skeptical on what's easier: create a new trait and extend it in an object, or just put your stuff under the same object. I personally like the latter more b/c it's just less code and fewer indirection and fewer symbols. Perhaps, we can revisit it when we have 10-20 instances and the grouping is the only vehicle to easier reasonability and discoverability.

This comment has been minimized.

@sergeykolbasov

sergeykolbasov Dec 5, 2018

Member

Shortly, EndpointModule[F[_]] is a container for all the aliases for Endpoint. methods such as get, post, path and so on. Otherwise user would neeed to type manually each time something like:

Endpoint.param[IO, String]("foo")

//equal to
import io.finch.catsEffect._
param("foo")

There are multiple ways to enable such syntax, one of them is to have the object extending EndpointModule[AnyEffect]. Details: https://github.com/finagle/finch/releases/tag/v0.25.0

This comment has been minimized.

@jcouyang

jcouyang Dec 5, 2018

Author Contributor

I'd prefer a separate file for this, ToEffect.scala

Hmm🤔 A second thought, if move to separate file, object mapper can simpliy extend the trait and has the ability to convert none effect to effect, which is more composable and less implicit

And type alias for F ~>E can be eliminated too, user don't have to import anything from Finch to implement F ~>E for their own type then

Not sure if it makes sense to you, I'll refactor it to demonstrate what I meant anyway

This comment has been minimized.

@jcouyang

jcouyang Dec 6, 2018

Author Contributor

@sergeykolbasov i see your point, still it's just few line of code and it's supported already, i'd not border creating a extra module just for monix

    import monix.eval.Task
    import monix.execution.Scheduler.Implicits.global
    val task = Endpoint[Task]
    import task._
    checkTaskValue((i: String) => get(zero) { Task(Ok(i)) })

as a user i'm more than happy in this way rather than install another module

This comment has been minimized.

@jcouyang

jcouyang Dec 6, 2018

Author Contributor

object mapper can simpliy extend the trait and has the ability to convert none effect to effect

forget it, that theory didn't work out

I'd prefer a separate file for this, ToEffect.scala

@vkostyukov just to be clear, i dont want to change back and forth again
IIUC is this what you saying?

  • combine IdEffect.scala and FutureToEffect.scala into ToEffect.scala

or

  • move everything including the trait ToEffect from package.scala into ToEffect.scala

This comment has been minimized.

@vkostyukov

vkostyukov Dec 6, 2018

Member

@jcouyang Both? A single file, ToEffect.scala that has an companion object will all these instances inlined.

import scala.util.{Failure, Success}

trait FutureToEffect {
implicit def twFutureToEffect[E[_]: Effect]: ToEffect[TwitterFuture, E] = new ToEffect[TwitterFuture, E] {

This comment has been minimized.

@sergeykolbasov

sergeykolbasov Dec 5, 2018

Member

I wonder if we can @deprecate those parameters to encourage users to upgrade their codebase to lawful effects.
At the moment deprecation warning appears if one uses one of the implicit Mappers for Future.

This comment has been minimized.

@vkostyukov

vkostyukov Dec 5, 2018

Member

Maybe we don't have to? Maybe deprecating these wasn't the good idea. These converters are pretty slim and going from Future to Effect turned out to be pretty straightforward. I'm thinking to keep them as is and revisit it later, after 1.0 or after we rework our mapper entirely.

This comment has been minimized.

@jcouyang

jcouyang Dec 5, 2018

Author Contributor

That's what I was thinking

}

object Mapper extends HighPriorityMapperConversions {
implicit def mapperFromEffectOutputHFunction[F[_] : Monad, A, B, FN, FOB](f: FN)(implicit

This comment has been minimized.

@vkostyukov

vkostyukov Dec 5, 2018

Member

@jcouyang ❤️ seeing all those red lines!

@vkostyukov
Copy link
Member

vkostyukov left a comment

This is great! Thanks a lot @jcouyang! Appreciate your patience with this.

Will merge it in after one more +1 from the maintainers.

@sergeykolbasov

This comment has been minimized.

Copy link
Member

sergeykolbasov commented Dec 7, 2018

LGTM

@vkostyukov vkostyukov merged commit cd43356 into finagle:master Dec 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vkostyukov

This comment has been minimized.

Copy link
Member

vkostyukov commented Dec 7, 2018

Thanks again @jcouyang! This is great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment