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

Unify backend builders, adding allocate to server builders #2240

Merged
merged 3 commits into from Nov 13, 2018

Conversation

Projects
None yet
3 participants
@rossabaker
Member

rossabaker commented Nov 3, 2018

Builds on #2239.

  • Introduces an internal BackendBuilder trait, which improves API cohesion through inheritance.
  • Puts the type parameter on the builder, not the resource/stream/allocate methods
  • Adds allocate to the servers, per popular request. I still don't like it.
  • Doesn't yet apply to JettyClient or AsyncHttpClient, which aren't builders

@rossabaker rossabaker added this to the 0.20.0 milestone Nov 3, 2018

@aeons

aeons approved these changes Nov 5, 2018

@rossabaker

This comment has been minimized.

Member

rossabaker commented Nov 5, 2018

I anticipate this might make @ChristopherDavenport slightly grumpy, so I'll hold off a little bit in case he wants to have an OOP fight.

import cats.effect.{Bracket, Resource}
import fs2.Stream
private[http4s] trait BackendBuilder[F[_], A] {

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Nov 5, 2018

Member

Why extend rather than just build instances of this like a typeclass. What does this extension buy us? resource => methods? Why not just expose that rather than this inheritance scheme?

This comment has been minimized.

@rossabaker

rossabaker Nov 5, 2018

Member

Pro: Traits are better for method discoverability. All of these methods will appear in the scaladoc and other primitive tooling.

Con: Typeclasses are better for ad hoc polymorphism. Someone could get the derived functions even if they didn't call the method resource. (But I'd frown at any such builder.)

I prioritized discoverability over extensibility in this context. If I saw scaladoc extension methods actually work, I'd probably flip.

This comment has been minimized.

@rossabaker

rossabaker Nov 5, 2018

Member

I left out two more cons.

The first is that it's more verbose. I don't really care, because that's our pain, not users':

trait BackendBuilder[F[_], A] {
  type R
  def resource(builder: A): Resource[F, R]
  def stream(builder: A): Stream[F, R] =
    Stream.resource(resource(builder))
  def allocate(builder: A)(implicit F: Bracket[F, Throwable]): F[(R, F[Unit])] =
    allocated(resource(builder))
}

object BackendBuilder {
  def apply[F[_], A](implicit ev: BackendBuilder[F, A]): BackendBuilder[F, A] = ev

  // These probably belong in org.http4s.implicits._
  implicit def backendBuilderSyntax[F[_], A](builder: A)(implicit A: BackendBuilder[F, A]) =
    new BackendBuilderOps(builder)
  class BackendBuilderOps[F[_], A](builder: A)(implicit val A: BackendBuilder[F, A]) {
    def resource: Resource[F, A.R] = A.resource(builder)
    def stream: Stream[F, A.R] = A.stream(builder)
    def allocate(implicit F: Bracket[F, Throwable]): F[(A.R, F[Unit])] = A.allocate(builder)
  }
}

The second issue is that it imposes an import tax on builder.stream and builder.allocate to get the syntax into scope. This is more pertinent to discovery than the scaladoc issue.

@rossabaker rossabaker merged commit f50130f into http4s:master Nov 13, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rossabaker rossabaker deleted the rossabaker:backend-builder branch Nov 13, 2018

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