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

Consider Reader-style instance for MonoidK of Kleisli #1928

Closed
SystemFw opened this issue Sep 25, 2017 · 17 comments
Closed

Consider Reader-style instance for MonoidK of Kleisli #1928

SystemFw opened this issue Sep 25, 2017 · 17 comments

Comments

@SystemFw
Copy link
Contributor

SystemFw commented Sep 25, 2017

The current instance of MonoidK for Kleisli (which I will refer to as Endo instance) looks like:

implicit def instance[F[_]](implicit M: Monad[F]): MonoidK[λ[α => Kleisli[F, α, α]]]

as opposed to what I'd call a Reader instance :

implicit def instance[F[_], A](implicit F: SemigroupK[F]): SemigroupK[Kleisli[F, A, ?]]

which imo should be preferred.

Concrete, driving use case
In http4s, we need to model an http service as Kleisli[OptionT[F, ?], Request, Response], and the proposed instance would allow composing services with <+>, getting the correct prioritised choice behaviour.

Rationale
An instance for Kleisli[A, ?] is more general than one for λ[α => Kleisli[F, α, α]], and therefore should be the instance, using a newtype for the other one. This is consistent with both Scalaz and Haskell, where A => B gets instances and A => A gets newtyped as Endo, and also internally consistent with the other instances for Kleisli, which are prevalently Reader style.

There is some discussion about this in #247, but I believe the use case in http4s is important enough to reconsider

@rossabaker

@SystemFw SystemFw changed the title Consider Reader-style instance for MonoidK[Kleisli] Consider Reader-style instance for MonoidK of Kleisli Sep 25, 2017
@kailuowang
Copy link
Contributor

cc @edmundnoble @ceedubs @non

@SystemFw
Copy link
Contributor Author

I can obviously open a PR as well if this gets approved

@tpolecat
Copy link
Member

Agree. The "EndoT" instance is interesting but I don't think it should be the default one. In scalaz I believe you get there via Endomorphic[Kleisli[F, ?, ?], A].

@rossabaker
Copy link
Member

Before http4s is used as the justification for this, @aeons is changing the HttpService type as described to ensure it works as well on the full project as it worked in my REPL. But I think this is the right thing to do regardless.

@ChristopherDavenport offered to implement it if we decide to proceed.

@LukaJCB
Copy link
Member

LukaJCB commented Sep 25, 2017

I also agree that the second instance is more useful in general. I'm a bit afraid of too many breaking changes in RC1 but I think this one should be quite minor and not a big issue for most :)

@kailuowang
Copy link
Contributor

The point of 1.0.0-MF is to identify breaking changes needed by the community before we go 1.0.

@johnynek
Copy link
Contributor

big +1 on this.

@kailuowang
Copy link
Contributor

Looks like we should proceed with it. @ChristopherDavenport still interested?

@rossabaker
Copy link
Member

@edmundnoble
Copy link
Contributor

I'm worried that we can't change this instance in place without changing behavior silently for users.

@johnynek
Copy link
Contributor

I think silently breaking users is possible but probably rare. Someone would have to have an endo which also has a MonoidK on the inner type, and that MonoidK would need to be in scope.

I'd be surprised if even one person will be in this boat.

@edmundnoble
Copy link
Contributor

Don't Either and Option fall under this? That seems common.

@ChristopherDavenport
Copy link
Member

ChristopherDavenport commented Sep 26, 2017

I'm interested, and will get an initial version up this evening.

@peterneyens
Copy link
Collaborator

I can update #1098 if needed?

@SystemFw
Copy link
Contributor Author

Yeah it seems like most of the code is already in #1098

@ChristopherDavenport
Copy link
Member

Cool, I can back down in favor of an existing PR.

@peterneyens
Copy link
Collaborator

Added in #1098

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

No branches or pull requests

9 participants