-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: add retry and http client plugins #671
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a question & a comment. Also, nit: there're several swiftlint warnings that should be addressed.
config.retryStrategyOptions = | ||
DefaultSDKRuntimeConfiguration<DefaultRetryStrategy, DefaultRetryErrorInfoProvider> | ||
.defaultRetryStrategyOptions | ||
} | ||
|
||
if var config = clientConfiguration as? DefaultHttpClientConfiguration { | ||
var httpClientConfiguration = DefaultSDKRuntimeConfiguration<DefaultRetryStrategy, DefaultRetryErrorInfoProvider> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, DefaultSDKRuntimeConfiguration
(which is one of the old config structs) has not been refactored (e.g., refactor to conform to the new ClientConfiguration
protocol, or delete and replace with the new SRA config) yet. Will the DefaultSDKRuntimeConfiguration
struct used here be refactored down the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the DefaultSDKRuntimeConfiguration struct used here be refactored down the line?
Yes. I will try to send out a PR later this week. I have refactored and included the changes in this PR.
@@ -81,19 +82,13 @@ open class HttpProtocolServiceClient( | |||
writer.openBlock("return ClientBuilder<\$L>(defaultPlugins: [", "])", serviceSymbol.name) { | |||
|
|||
val defaultPlugins: MutableList<Plugin> = mutableListOf(DefaultClientPlugin()) | |||
val customPlugins: MutableList<Plugin> = mutableListOf() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: What fell under customPlugins
, and why is the codegen for it deleted now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There weren't any plugins that fell under custom plugins. It should only be default plugins.
555e17c
to
d1adea7
Compare
writer.write("let config = try \$L(\"\$L\", \"\$L\")", serviceConfig.typeName, serviceName, serviceSymbol.name) | ||
writer.write("let config = try \$L()", serviceConfig.typeName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change 👍 . Are these set anywhere else right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need swiftlint warnings (10) taken care of, should be good afterwards.
d1adea7
to
29d1bae
Compare
29d1bae
to
d553ad3
Compare
Description of changes
Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.