async-http-client icon indicating copy to clipboard operation
async-http-client copied to clipboard

HTTPClient.Configuration.init parameter `redirectConfiguration` inconsistently named and suboptimal type

Open weissi opened this issue 5 years ago • 2 comments

This is quite a minor issue but redirectConfiguration sticks out:

public init(tlsConfiguration: TLSConfiguration? = nil,
            redirectConfiguration: RedirectConfiguration? = nil,
            timeout: Timeout = Timeout(),
            proxy: Proxy? = nil,
            ignoreUncleanSSLShutdown: Bool = false,
            decompression: Decompression = .disabled) {

When I'm initialising a Configuration, I know that I'd be configuring the redirect configuration. I think it should be called redirects: or something similar.

Also, the type isn't great. It shouldn't be optional.

See Xcode's completion (which is accurate):

Screenshot 2020-04-05 at 20 46 56

You'd expect that .none and .disallow are the same but .none secretly means "default" (which is not .disallow). The parameter should be non-optional and default to a new value .default.

We should deprecate this init and also fix the other optional ones.

weissi avatar Apr 05 '20 19:04 weissi

This is definitely not optimal. In terms of naming, I was following TLSConfiguration example, should we rename this as well? Optionals are definitely should be replaced with default values, yes, but this will be API-breaking change.

artemredkin avatar Apr 26 '20 10:04 artemredkin

@artemredkin we can deprecate this .init and make a new one that is

public init(tls: TLSConfiguration = .forClient,
            allowRedirects: RedirectConfiguration = .none,
            timeout: Timeout = Timeout(),
            proxy: Proxy? = nil,
            ignoreUncleanSSLShutdown: Bool = false,
            decompression: Decompression = .disabled) {

and deprecate the old one?

weissi avatar Apr 29 '20 14:04 weissi