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

Bind and BindAndHandle handles remote address in the same way now (#344). Refactoring to decrease code duplication. #442

Merged
merged 1 commit into from Oct 31, 2016

Conversation

@rbudzko
Copy link
Contributor

commented Oct 27, 2016

Additionally, I've "borrowed" @jrudolph tests.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Oct 27, 2016

Can one of the repo owners verify this patch?

@ktoso

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

OK TO TEST

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Oct 27, 2016

Test FAILed.

@2beaucoup
Copy link
Contributor

left a comment

Nice cleanups @rbudzko!

@@ -21,4 +21,6 @@ private[akka] object HttpAttributes {
private[akka] def remoteAddress(address: Option[InetSocketAddress]) =
Attributes(RemoteAddress(address))

private[akka] def empty =

This comment has been minimized.

Copy link
@2beaucoup

2beaucoup Oct 27, 2016

Contributor

switch to val?

This comment has been minimized.

Copy link
@jrudolph

jrudolph Oct 27, 2016

Member

Do we private[akka] all the things if the outer scope is private[akka] already?

@@ -57,7 +56,12 @@ class HttpExt(private val config: Config)(implicit val system: ActorSystem) exte

private[this] final val DefaultPortForProtocol = -1 // any negative value

private def fuseServerLayer(settings: ServerSettings, connectionContext: ConnectionContext, log: LoggingAdapter)(implicit mat: Materializer): BidiFlow[HttpResponse, ByteString, ByteString, HttpRequest, NotUsed] = {
private[this]type ServerLayerBidiFlow = BidiFlow[HttpResponse, ByteString, ByteString, HttpRequest, NotUsed]
private[this]type ServerLayerFlow = Flow[ByteString, ByteString, Future[Done]]

This comment has been minimized.

Copy link
@2beaucoup

2beaucoup Oct 27, 2016

Contributor

space before "type" (2x). is the "[this]" needed here?

@@ -21,4 +21,6 @@ private[akka] object HttpAttributes {
private[akka] def remoteAddress(address: Option[InetSocketAddress]) =
Attributes(RemoteAddress(address))

private[akka] def empty =

This comment has been minimized.

Copy link
@viktorklang
settings.timeouts.idleTimeout
)

private def chosePort(port: Int, connectionContext: ConnectionContext) = if (port >= 0) port else connectionContext.defaultPort

This comment has been minimized.

Copy link
@jrudolph

jrudolph Oct 27, 2016

Member

"choosePort"

@jrudolph

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

Very nice, indeed. Things get a bit crowded in Http.scala, so we might want to move some of the private methods somewhere else in the future.

@jrudolph

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

PLS BUILD

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Oct 27, 2016

Test FAILed.

@akka-ci akka-ci removed the validating label Oct 27, 2016

Bind and BindAndHandle handles remote address in the same way now (#344
…). Refactoring to decrease code duplication.
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Oct 27, 2016

Test PASSed.

@rbudzko

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2016

Fixed quirks pointed out.

@jrudolph

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

LGTM. @2beaucoup can you resolve your review?

@2beaucoup

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

Sure. Done.

@jrudolph

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

LGTM. Thanks a lot @rbudzko.

@jrudolph jrudolph merged commit e54b9e7 into akka:master Oct 31, 2016

2 checks passed

default Test PASSed.
Details
typesafe-cla-validator All users have signed the CLA
Details

@rbudzko rbudzko deleted the rbudzko:feature/bind branch Oct 31, 2016

@ktoso

This comment has been minimized.

Copy link
Member

commented Nov 2, 2016

Thanks a lot @rbudzko :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.