Introduce a ConnectionTarget enum
#501
Conversation
|
OK, I just did it and moved it up in to |
karwa
force-pushed
the
a-good-host
branch
2 times, most recently
from
0a7f3f7 to
2b58022
Compare
|
One idea I'd like to float is potentially naming this Also, it would avoid overloading the term "host" too much. It already clashes with |
|
|
|
OK, I've changed it to |
|
Did a quick read through the changes and it is a nice cleanup. Thanks! Will do a proper review on monday. |
There was a problem hiding this comment.
Great patch! Looks good to me modulo one nit. We should still wait for @fabianfett review until we merge it.
w.r.t.:
The async redesign simplifies Request somewhat, but I couldn't see any mention of hosts or hostnames on the new request type. One idea worth exploring would be making this public API as part of that redesign.
The new Request type, called HTTPClientRequest, does not do any parsing or validation. It also just accepts a url as a String. Parsing, destructering and validation of the url is all delayed until execution. The advantage is that the initialiser of HTTPClientRequest is non-throwing and all properties are vars. Therefore we will not even have ConnectionTarget as an internal property on HTTPClientRequest.
My plan is to introduce a new internal type called PreparedRequest which is created as part of executing the request. This type will be the new home for all properties which are computed as part of the current initialiser of HTTPClient.Request and the validation step which is currently also executed before sending out a HTTPClient.Request.
| path = self.unixPath | ||
| } else { | ||
| path = "\(self.host):\(self.port)" | ||
| var hostDescription = "" |
There was a problem hiding this comment.
nit: Swift should see that we set hostDescription exactly once on every code path. If we remove the default value "" we should therefore be able to make this a let.
| @@ -45,6 +45,30 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand | |||
| return self.proxyEstablishedPromise?.futureResult | |||
| } | |||
|
|
|||
| convenience | |||
| init(target: ConnectionTarget, | |||
There was a problem hiding this comment.
Minor nit: I'd like to put convenience init on the same line.
|
@karwa I merge it now and fix the nits in a follow up PR quickly because I want to use them in another PR. Thanks again for this patch! |
|
Thanks @dnadoba! I was planning to fix in the evening yesterday, but was too tired 😴 |
I added this as part of the WebURL port, because it can provide specific host kinds and doesn't need to re-parse the hostname, but I think it is generally a useful thing to model.
Currently,
ConnectionPool.Keystores ahost,port, andsocketPathstring, even though only one of (host + port) and (socketPath) can be populated. This lends itself well to an enum. Additionally, we can extract what kind of host we're dealing with, so we don't need to re-parse the hostname later.One thing I'd like comments on is whether this should perhaps be moved up to
Request. This would also allows us to stop pretending that unix domain sockets have ports. As I see it, this should really beRequest's business; a part of how it extracts information from the URL should be parsing the hostname string (perhaps using APIs from the URL type, if it provides that).The async redesign simplifies
Requestsomewhat, but I couldn't see any mention of hosts or hostnames on the new request type. One idea worth exploring would be making this public API as part of that redesign.