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
Switch to static builder()
methods
#2221
Conversation
ClientFactory DEFAULT = new ClientFactoryBuilder().build(); | ||
ClientFactory DEFAULT = builder().build(); |
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.
Should we switch to ofDefault()
?
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.
+1 I would lean towards using ofDefaults()
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.
OK. Let me take care of it in another pull request.
While working on this, I found the following builder classes do not match well with the new pattern:
|
...rc/main/java/com/linecorp/armeria/client/retrofit2/RetrofitMeterIdPrefixFunctionBuilder.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #2221 +/- ##
============================================
+ Coverage 73.52% 73.55% +0.02%
- Complexity 9720 9749 +29
============================================
Files 848 849 +1
Lines 37535 37555 +20
Branches 4610 4610
============================================
+ Hits 27599 27624 +25
+ Misses 7575 7568 -7
- Partials 2361 2363 +2
Continue to review full report at Codecov.
|
+1 for
Retrofit is a final class so we cannot extend it. I'm not sure which one is the best. 😢 |
Gentle ping, guys 😆 |
Took advantage of review lag to migrate more builders. 😝 |
Note to self: Use |
core/src/main/java/com/linecorp/armeria/client/ClientOptions.java
Outdated
Show resolved
Hide resolved
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 work! 👍
core/src/main/java/com/linecorp/armeria/client/ClientRequestContext.java
Outdated
Show resolved
Hide resolved
6d61a55
to
92aa64b
Compare
Related: line#1719 Motivation: We decided to switch from `new *Builder()` to `*.builder()`. Modifications: - Switch to static `builder()` methods in: - `ClientFactory` - `ClientOptions` - `ClientDecoration` - `ClientConnectionTimings` - `HttpClient` - `LoggingClient` - `LoggingService` - `EndpointInfo` - `FieldInfo` - `ClientCacheControl` - `ServerCacheControl` - `ClientRequestContext` - `ServiceRequestContext` - `RequestContextCurrentTraceContext` - `RetrofitMeterIdPrefixFunction` - (Breaking) `RetrofitMeterIfPrefixFunctionBuilder` is now a top-level class without a public constructor. - Miscellaneous: - Increase the test timeout for `ArmeriaSpringActuatorAutoConfigurationTest` for less flakiness - Maybe fixes the flakiness in `GracefulShutdownSupportTest`
Sneaking in two miscellaneous changes, but I guess it's OK. Will merge tomorrow morning. Leave a message if you'd like them as a separate PR. |
Related: line#1719 Motivation: We decided to switch from `new *Builder()` to `*.builder()`. Modifications: - Switch to static `builder()` methods in: - `ClientFactory` - `ClientOptions` - `ClientDecoration` - `ClientConnectionTimings` - `HttpClient` - `LoggingClient` - `LoggingService` - `EndpointInfo` - `FieldInfo` - `ClientCacheControl` - `ServerCacheControl` - `ClientRequestContext` - `ServiceRequestContext` - `RequestContextCurrentTraceContext` - `RetrofitMeterIdPrefixFunction` - (Breaking) `RetrofitMeterIfPrefixFunctionBuilder` is now a top-level class without a public constructor. - Miscellaneous: - Increase the test timeout for `ArmeriaSpringActuatorAutoConfigurationTest` for less flakiness - Maybe fixes the flakiness in `GracefulShutdownSupportTest`
Related: #2221 * Makes user to use ServerListener.builder() to get ServerListenerBuilder * Makes user to use SimpleCompositeService.builder() to get SimplecompositeServiceBuilder * Makes user to use SimpleCompositeRpcService.builder() to get SimplecompositeRpcServiceBuilder * Makes user to use CorsPolicy.builder() to get CorsPolicyBuilder * Makes user to use DocService.builder() to get DocServiceBuilder * Makes user to use LoggingService.builder() to get LoggingServiceBuilder * Makes user to use JettyService.builder() to get JettyServiceBuilder * Makes user to use SamlServiceProvider.builder() to get SamlServiceProviderBuilder * Makes user to use ZooKeeperUpdatingListener.builder() to get ZooKeeperUpdatingListenerBuilder
Related: line#1719 Motivation: We decided to switch from `new *Builder()` to `*.builder()`. Modifications: - Switch to static `builder()` methods in: - `ClientFactory` - `ClientOptions` - `ClientDecoration` - `ClientConnectionTimings` - `HttpClient` - `LoggingClient` - `LoggingService` - `EndpointInfo` - `FieldInfo` - `ClientCacheControl` - `ServerCacheControl` - `ClientRequestContext` - `ServiceRequestContext` - `RequestContextCurrentTraceContext` - `RetrofitMeterIdPrefixFunction` - (Breaking) `RetrofitMeterIfPrefixFunctionBuilder` is now a top-level class without a public constructor. - Miscellaneous: - Increase the test timeout for `ArmeriaSpringActuatorAutoConfigurationTest` for less flakiness - Maybe fixes the flakiness in `GracefulShutdownSupportTest`
) Related: line#2221 * Makes user to use ServerListener.builder() to get ServerListenerBuilder * Makes user to use SimpleCompositeService.builder() to get SimplecompositeServiceBuilder * Makes user to use SimpleCompositeRpcService.builder() to get SimplecompositeRpcServiceBuilder * Makes user to use CorsPolicy.builder() to get CorsPolicyBuilder * Makes user to use DocService.builder() to get DocServiceBuilder * Makes user to use LoggingService.builder() to get LoggingServiceBuilder * Makes user to use JettyService.builder() to get JettyServiceBuilder * Makes user to use SamlServiceProvider.builder() to get SamlServiceProviderBuilder * Makes user to use ZooKeeperUpdatingListener.builder() to get ZooKeeperUpdatingListenerBuilder
Related: #1719
Motivation:
We decided to switch from
new *Builder()
to*.builder()
.Modifications:
builder()
methods in:ClientFactory
ClientOptions
ClientDecoration
ClientConnectionTimings
HttpClient
LoggingClient
LoggingService
EndpointInfo
FieldInfo
ClientCacheControl
ServerCacheControl
ClientRequestContext
ServiceRequestContext
RequestContextCurrentTraceContext
RetrofitMeterIdPrefixFunction
RetrofitMeterIfPrefixFunctionBuilder
is now a top-level class without a public constructor.ArmeriaSpringActuatorAutoConfigurationTest
for less flakinessGracefulShutdownSupportTest