add a separate configuration struct for pool #284
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lukasa
requested changes
Jul 17, 2020
| @@ -860,6 +860,16 @@ extension HTTPClient.Configuration { | |||
| /// - warning: Cycle detection will keep all visited URLs in memory which means a malicious server could use this as a denial-of-service vector. | |||
| public static func follow(max: Int, allowCycles: Bool) -> RedirectConfiguration { return .init(configuration: .follow(max: max, allowCycles: allowCycles)) } | |||
| } | |||
|
|
|||
| /// Connection pool configuration. | |||
| public struct PoolConfiguration { | |||
There was a problem hiding this comment.
Can we make this Hashable?
There was a problem hiding this comment.
Out of curiosity, why do you think we might need it? Also, TimeAmount is not hashable, so this will require implementing hash(into:), should I add it?
There was a problem hiding this comment.
TimeAmount is Hashable as of 2.19.0.
There was a problem hiding this comment.
As to why we need it, it's not so much that we need it as that, for trivial structures, missing Equatable and Hashable conformances tend to be minor annoyances. It's nicer just to have them.
Lukasa
reviewed
Jul 17, 2020
| /// Connection pool configuration. | ||
| public struct PoolConfiguration: Hashable { | ||
| // Specifies amount of time connections are kept idle in the pool. | ||
| var idleTimeout: TimeAmount |
There was a problem hiding this comment.
Whoops, nearly missed this.
Suggested change
| var idleTimeout: TimeAmount | |
| public var idleTimeout: TimeAmount |
Lukasa
approved these changes
Jul 17, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move pool idle time configuration to separate struct.
Motivation:
Right now there is only one configuration parameter for the pool, idle time, but we might have more in the future. I think it makes sense to wrap all pool-related setting in a separate struct.
Modifications:
Add PoolConfiguration struct and move idle time to that struct
Result:
All pool level settings will be in one namespace.