Skip to content

HTTPClient.Configuration.init parameter redirectConfiguration inconsistently named and suboptimal type #197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
weissi opened this issue Apr 5, 2020 · 2 comments
Labels
kind/enhancement Improvements to existing feature.
Milestone

Comments

@weissi
Copy link
Contributor

weissi commented Apr 5, 2020

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 weissi changed the title HTTPClient.Configuration.init parameter redirectConfiguration badly named and bad type HTTPClient.Configuration.init parameter redirectConfiguration inconsistently named and suboptimal type Apr 5, 2020
@artemredkin
Copy link
Collaborator

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 artemredkin added the kind/enhancement Improvements to existing feature. label Apr 26, 2020
@artemredkin artemredkin added this to the 2.0.0 milestone Apr 26, 2020
@artemredkin artemredkin added the ⚠️ semver/major Breaks existing public API. label Apr 26, 2020
@weissi
Copy link
Contributor Author

weissi commented Apr 29, 2020

@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 weissi removed the ⚠️ semver/major Breaks existing public API. label Apr 29, 2020
@artemredkin artemredkin modified the milestones: 2.0.0, 1.2.0 May 20, 2020
@weissi weissi mentioned this issue May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements to existing feature.
Projects
None yet
Development

No branches or pull requests

2 participants