Refactor URL component extraction #485
Conversation
|
Can one of the admins verify this patch? |
6 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
@swift-server-bot test this please |
|
@swift-server-bot add to allowlist |
| self.socketPath = try self.kind.socketPathFromURL(url) | ||
| self.uri = self.kind.uriFromURL(url) | ||
|
|
||
| (self.kind, self.host, self.socketPath, self.uri) = try Self.deconstructURL(url) |
There was a problem hiding this comment.
Great work! Thank you!
Would you mind returning the scheme and port in deconstructURL(_:) as well?
scheme is already a stored property in HTTPClient.Request but port not.
async-http-client/Sources/AsyncHTTPClient/HTTPHandler.swift
Lines 266 to 268 in 5dfbba8
However, I don't mind adding
port as another stored property. This would allow us to better reuse this functionality in the upcoming new HTTPClientRequest for async/await support as described in this proposal:https://github.com/swift-server/async-http-client/blob/96872470475f18a5667fbf681e2e0f67ad8a0110/docs/async-await.md#new-httpclientrequest-type
There was a problem hiding this comment.
@karwa friendly ping 🙂
I would like to use your changes soon. Do you have any estimate when you have time to work on this again?
There was a problem hiding this comment.
Sure, I'll address the comments now and update the patch
Co-authored-by: Fabian Fett <[email protected]>
This is currently split across 4 functions (well, 3 functions and an initialiser). It's much easier to understand how AHC understands these URL formats if it happens in a single function (especially since schemes like
http+unixare not standardised).Additionally, this rejects empty HTTP hostnames, as they are invalid per the spec, and empty socket paths, since they are meaningless.