Extend swift-configuration support to proxy configuration#904
Extend swift-configuration support to proxy configuration#904hamzahrmalik wants to merge 2 commits into
Conversation
| /// Only supported for `http` proxies. | ||
| /// | ||
| /// - Throws: `HTTPClientError.invalidProxyConfiguration` if `enabled` is `true` but `host` is missing, `type` is unknown, | ||
| /// `port` is missing for an HTTP proxy, or authorization is specified for a SOCKS proxy. |
There was a problem hiding this comment.
nit:
| /// `port` is missing for an HTTP proxy, or authorization is specified for a SOCKS proxy. | |
| /// `port` is missing for an HTTP proxy, or `authorization` is specified for a SOCKS proxy. |
Also, I know the HTTPClient.Authorization config init has this documented, but it might be worth calling out here that this error is also thrown when the provided authorization for an HTTP proxy is invalid.
| guard configReader.bool(forKey: "enabled", default: false) else { | ||
| return nil | ||
| } | ||
| guard let host = configReader.string(forKey: "host") else { |
There was a problem hiding this comment.
Are you deliberately not using requiredString(forKey:) so that you can thrown an HTTPClientError? (Ignore me if I'm misremembering the API.)
Minor concern is that invalidProxyConfiguration doesn't tell you what you got wrong, only that you got something wrong. I assume the swift-configuration error would tell you what required key was missing.
There was a problem hiding this comment.
there's no precedent within this file, because we dont have any required props until now
There is invalidHTTPVersionConfiguration and invalidDNSOverridesConfiguration but these are for invalid values rather than missing values.
I think youre right and using the requiredString etc functions makes more sense
There was a problem hiding this comment.
you do still need the custom error for Authentication though, since we need one of several keys to exist
3 participants
Extending #878 to also allow configuring the proxy via ConfigReader