Add swift-configuration support #878
Conversation
Sources/AsyncHTTPClient/HTTPClientConfiguration+SwiftConfiguration.swift
Show resolved
Hide resolved
| public init(configReader: ConfigReader) throws { | ||
| self.init() | ||
|
|
||
| // Each entry in the list should be a colon separated pair e.g. localhost:127.0.0.1 or localhost:::1 |
There was a problem hiding this comment.
colon felt more natural to me for defining key-value pairs. I don't mind though
There was a problem hiding this comment.
comma would probably make the ipv6 case slightly eaiser to parse too
There was a problem hiding this comment.
Oh right, comma might be used for splitting the pairs themselves e.g. in env vars. Hmm then comma isn't great, but colon is a bit confusing.
|
lgtm, I'll let the maintainers actually approve though 🙂 |
| /// - `dnsOverrides` (string array, optional): Colon-separated host:IP pairs for DNS overrides (e.g., "localhost:127.0.0.1"). | ||
| /// - `redirect` (scoped): Redirect handling configuration read by ``RedirectConfiguration/init(configReader:)``. | ||
| /// - `timeout` (scoped): Timeout configuration read by ``Timeout/init(configReader:)``. | ||
| /// - `connectionPool` (scoped): Connection pool configuration read by ``ConnectionPool/init(configReader:)``. | ||
| /// - `httpVersion` (string, optional, default: automatic): HTTP version to use ( "automatic" or "http1Only"). | ||
| /// - `maximumUsesPerConnection` (int, optional, default: nil, no limit): Maximum uses per connection. |
There was a problem hiding this comment.
Should we do camelcase or dot separated? In Temporal we choose the latter https://github.com/apple/swift-temporal-sdk/blob/eb356f25b53d4ac50b1505dea1b7306ba7296bc8/Sources/Temporal/Client/TemporalClient%2BConfiguration.swift#L111
There was a problem hiding this comment.
I used dots for hierarchy but camel case for words
There was a problem hiding this comment.
dns.overrides and http.version make sense in that system. But maximum.uses.per.connection does not
There was a problem hiding this comment.
This could be connection.limits.maximum.uses. It is not the end of the world but it would be great if we could be a bit consistent around the ecosystem. @czechboy0 WDYT?
There was a problem hiding this comment.
oh yeah i like that idea
There was a problem hiding this comment.
I've been doing the same as Hamzah - use dots for separating components, with individual components being camelCase. So http.maxConnectionCount, for example.
There was a problem hiding this comment.
Should we document this somewhere? It feels like that would be good to have a recommendation at least for the ecosystem.
There was a problem hiding this comment.
Feel free to add more questions to the issue, will add docs on this
| let testProvider = InMemoryProvider(values: [ | ||
| "dnsOverrides": .init(.stringArray(["invalidentry", "localhost:127.0.0.1"]), isSecret: false) | ||
| ]) |
There was a problem hiding this comment.
what about spaces:
localhost: 127.0.0.1
Are we fine with the silent error here?
There was a problem hiding this comment.
i will make it trim spaces, i think that's going to be the least surprising behaviour
There was a problem hiding this comment.
do we think it's preferable to throw an error on invalid config? It would make it much more clear i suppose
There was a problem hiding this comment.
Yeah I think throwing an error if the value of a key isn't valid is very reasonable
| @Test | ||
| @available(macOS 15.0, iOS 18.0, watchOS 11.0, tvOS 18.0, visionOS 2.0, *) | ||
| func singleDnsOverride() throws { | ||
| let testProvider = InMemoryProvider(values: [ | ||
| "dnsOverrides": .init(.stringArray(["api.example.com:10.0.0.1"]), isSecret: false) | ||
| ]) | ||
|
|
||
| let configReader = ConfigReader(provider: testProvider) | ||
| let config = try HTTPClient.Configuration(configReader: configReader) | ||
|
|
||
| #expect(config.dnsOverride["api.example.com"] == "10.0.0.1") | ||
| #expect(config.dnsOverride.count == 1) | ||
| } |
There was a problem hiding this comment.
what's the point of this test? what is tested here, that the other tests miss?
There was a problem hiding this comment.
it's not needed. i think i wrote this one first but the other ones cover this and more so i'll remove it
| @@ -0,0 +1,124 @@ | |||
| // swift-tools-version:6.0 | |||
There was a problem hiding this comment.
Is there a way without duplicating, all the Package.swifts?
There was a problem hiding this comment.
not that i know of
|
Am I correctly assuming, that this new behavior is only available to newly created clients and not the default shared one? If yes, is this the intended behavior? |
The changes in this PR allow you to create a configuration from a configuration reader You can then use that configuration to make a client The shared client does not support a custom configuration. Therefore you are correct that the changes in this PR do not apply to the singleton |
I think this is expected for now. We could think about letting the singleton client use the |
4 participants
Users should be able to use swift-configuration to create the http client configuration object
Changes