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

Blaze Builder Switch #2054

Merged
merged 7 commits into from Sep 6, 2018

Conversation

Projects
None yet
2 participants
@ChristopherDavenport
Member

ChristopherDavenport commented Sep 6, 2018

Switch BlazeBuilder to HttpApp. Can either get stolen or merged after #1993

@@ -63,7 +66,7 @@ class BlazeBuilder[F[_]](
isHttp2Enabled: Boolean,
maxRequestLineLen: Int,
maxHeadersLen: Int,
serviceMounts: Vector[ServiceMount[F]],
serviceMount: HttpApp[F],

This comment has been minimized.

@rossabaker

rossabaker Sep 6, 2018

Member

I'd call this HttpApp. The type broke anyway, and the name "service" has fallen out of fashion.

}
}
copy(serviceMounts = serviceMounts :+ ServiceMount[F](prefixedService, prefix))
def mountService(service: HttpApp[F]): Self = {

This comment has been minimized.

@rossabaker

rossabaker Sep 6, 2018

Member

withService would be more idiomatic with the way the builders are trending.

@@ -174,7 +168,7 @@ class BlazeBuilder[F[_]](
copy(banner = banner)
def start: F[Server[F]] = F.delay {
val aggregateService = Router(serviceMounts.map(mount => mount.prefix -> mount.service): _*)
// val aggregateService = Router(serviceMounts.map(mount => mount.prefix -> mount.service): _*)

This comment has been minimized.

@rossabaker

rossabaker Sep 6, 2018

Member

Kill it

@@ -61,7 +60,7 @@ private[blaze] class Http1ServerStage[F[_]](
// micro-optimization: unwrap the routes and call its .run directly
private[this] val routesFn = routes.run
private[this] val optionTSync = Sync[OptionT[F, ?]]
// private[this] val optionTSync = Sync[OptionT[F, ?]]

This comment has been minimized.

@rossabaker

rossabaker Sep 6, 2018

Member

Kill it

serviceErrorHandler: ServiceErrorHandler[F])(implicit F: Effect[F])
extends TailStage[StreamFrame] {
// micro-optimization: unwrap the service and call its .run directly
private[this] val serviceFn = service.run
private[this] val optionTSync = Sync[OptionT[F, ?]]
// private[this] val optionTSync = Sync[OptionT[F, ?]]

This comment has been minimized.

@rossabaker

rossabaker Sep 6, 2018

Member

Kill it

@@ -15,7 +15,7 @@ import scala.concurrent.duration.Duration
private[blaze] object ProtocolSelector {
def apply[F[_]: ConcurrentEffect](
engine: SSLEngine,
routes: HttpRoutes[F],
routes: HttpApp[F],

This comment has been minimized.

@rossabaker

rossabaker Sep 6, 2018

Member

app or httpApp. routes is really misleading.

import org.specs2.specification.AfterAll
import scala.io.Source
class BlazeServerSpec extends Http4sSpec with AfterAll {

This comment has been minimized.

@rossabaker

rossabaker Sep 6, 2018

Member

In time, we could probably make ServerSpec take a Resource[IO, Server[F]] and reunite, but let's clean that up when we clean up the servlet builders.

@@ -33,7 +33,7 @@ class Http1ServerStageSpec extends Http4sSpec {
def runRequest(
req: Seq[String],
routes: HttpRoutes[IO],
routes: HttpApp[IO],

This comment has been minimized.

@rossabaker

rossabaker Sep 6, 2018

Member

I've been changing legacy names in tests, too, but I care less about them since they're not public API.

@rossabaker

Don't merge yet, of course, but this looks good.

Alternative approach I just thought of: name this BlazeServerBuilder (for harmony with BlazeClientBuilder), leave the old one that works, but deprecated, so we can nudge people along.

If we make the new one BlazeServerBuilder, we'd probably want to invest in bringing it up to the new standard, but you've got a good start on it already.

@rossabaker

I like it, with future effort to express the old in terms of the new.

This is also going to have tut fallout.

@rossabaker rossabaker added this to the 0.19.0-M2 milestone Sep 6, 2018

@ChristopherDavenport ChristopherDavenport merged commit ce02777 into http4s:master Sep 6, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ChristopherDavenport ChristopherDavenport deleted the ChristopherDavenport:blazeBuilderSwitch branch Sep 6, 2018

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