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

renamed from (deprecated) service to kleisli #2613

Merged
merged 3 commits into from Jun 10, 2019

Conversation

@TomTriple
Copy link
Contributor

@TomTriple TomTriple commented Jun 1, 2019

No description provided.

Co-Authored-By: Diego E. Alonso Blas <diesalbla@gmail.com>
@@ -8,8 +8,8 @@ import cats.data.{Kleisli, OptionT}

trait KleisliSyntax {
implicit def http4sKleisliResponseSyntax[F[_]: Functor, A](
Copy link
Member

@rossabaker rossabaker Jun 3, 2019

Choose a reason for hiding this comment

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

Aside: maybe we should have had optiont in the name here in case we had syntax for other transformers. The parametric A is what prevents it from being just HttpRoutes.

Loading

Copy link
Contributor Author

@TomTriple TomTriple Jun 3, 2019

Choose a reason for hiding this comment

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

Renaming breaks a test in BlazeClientSpec, so I removed that again ;)

Loading

Copy link
Member

@rossabaker rossabaker Jun 3, 2019

Choose a reason for hiding this comment

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

BlackClientSpec is regrettably flaky on Travis CI, so unless it broke locally, I'll bet it wasn't you who broke it.

Loading

Copy link
Contributor Author

@TomTriple TomTriple Jun 4, 2019

Choose a reason for hiding this comment

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

I'll have a look at it again when I'm back home

Loading

Copy link
Contributor Author

@TomTriple TomTriple Jun 4, 2019

Choose a reason for hiding this comment

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

You won your bet, it worked locally!

Loading

@@ -8,8 +8,8 @@ import cats.data.{Kleisli, OptionT}

trait KleisliSyntax {
implicit def http4sKleisliResponseSyntax[F[_]: Functor, A](
service: Kleisli[OptionT[F, ?], A, Response[F]]): KleisliResponseOps[F, A] =
new KleisliResponseOps[F, A](service)
kleisli: Kleisli[OptionT[F, ?], A, Response[F]]): KleisliResponseOps[F, A] =
Copy link
Member

@rossabaker rossabaker Jun 3, 2019

Choose a reason for hiding this comment

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

It's technically a source-breaking change to change a parameter name, but since @deprecatedName is deprecated between 2.12 and 2.13, and this is used only implicitly, I'm not inclined to worry about it.

Loading

Copy link
Contributor Author

@TomTriple TomTriple Jun 3, 2019

Choose a reason for hiding this comment

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

Interesting to know @rossabaker ! I did not think for a second about that.

Loading

Copy link
Member

@rossabaker rossabaker Jun 3, 2019

Choose a reason for hiding this comment

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

This is something to be more careful of on things like case classes, or functions and constructors that take a few arguments. Particularly when functions take more than one argument of the same type, people are likely to call things with named parameters. But yeah, it's rare for people to call by name on unary functions, and even rarer on implicits.

Loading

@TomTriple TomTriple force-pushed the feature/rename-to-kleisli branch from 24085fa to a11e40d Jun 3, 2019
@rossabaker rossabaker merged commit e036356 into http4s:master Jun 10, 2019
1 check passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants