Skip to content
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

Simplify decorator builder methods #1484

Merged
merged 1 commit into from Dec 11, 2018

Conversation

trustin
Copy link
Member

@trustin trustin commented Dec 7, 2018

Motivation:

Practically, there are only 2 ways of adding a client decorator:

  • builder.decorator(HttpRequest.class, HttpResposne.class, ...);
  • builder.decorator(RpcRequest.class, RpcResponse.class, ...);

Instead of requiring a user to specify req/res types, we could just
provide two builder methods dedicated to each case.

Modifications:

  • Deprecated the old decorator() builder methods.
  • Added decorator(Function) and decorator(ClientDecoratorFunction)
    which add an HTTP-level decorator.
  • Added rpcDecorator(Function) and rpcDecorator(ClientDecoratorFunction)
    which add an RPC-level decorator.
  • Removed HttpClient.decorator() methods which became unnecessary due
    to the new decorator() methods in AbstractClientOptionsBuilder.

Result:

@trustin trustin added this to the 0.77.0 milestone Dec 7, 2018
@trustin trustin force-pushed the cleaner_decorator_api branch 2 times, most recently from 499ee3a to df141b7 Compare December 7, 2018 03:13
Motivation:

Practically, there are only 2 ways of adding a client decorator:

- `builder.decorator(HttpRequest.class, HttpResposne.class, ...);`
- `builder.decorator(RpcRequest.class, RpcResponse.class, ...);`

Instead of requiring a user to specify req/res types, we could just
provide two builder methods dedicated to each case.

Modifications:

- Deprecated the old `decorator()` builder methods.
- Added `decorator(Function)` and `decorator(ClientDecoratorFunction)`
  which adds an HTTP-level decorator.
- Added `rpcDecorator(Function)` and `rpcDecorator(ClientDecoratorFunction)`
  which adds an RPC-level decorator.
- Removed `HttpClient.decorator()` methods which became unnecessary due
  to the new `decorator()` methods in `AbstractClientOptionsBuilder`.

Result:

- Fixes line#1482
- Simpler builder API
- Leaner user code
@codecov
Copy link

codecov bot commented Dec 7, 2018

Codecov Report

Merging #1484 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1484      +/-   ##
============================================
+ Coverage     72.17%   72.18%   +0.01%     
- Complexity     6648     6657       +9     
============================================
  Files           637      637              
  Lines         27575    27591      +16     
  Branches       3319     3319              
============================================
+ Hits          19901    19916      +15     
- Misses         5924     5928       +4     
+ Partials       1750     1747       -3
Impacted Files Coverage Δ Complexity Δ
...ia/client/limit/ConcurrencyLimitingHttpClient.java 100% <ø> (ø) 7 <0> (ø) ⬇️
.../armeria/client/metric/MetricCollectingClient.java 100% <ø> (ø) 4 <0> (ø) ⬇️
...com/linecorp/armeria/client/HttpClientBuilder.java 80% <ø> (-2.36%) 6 <0> (-2)
...main/java/com/linecorp/armeria/client/Clients.java 66.07% <ø> (ø) 16 <0> (ø) ⬇️
...necorp/armeria/client/ClientDecorationBuilder.java 100% <100%> (ø) 15 <6> (+6) ⬆️
...p/armeria/client/AbstractClientOptionsBuilder.java 87.83% <100%> (-4.59%) 24 <5> (+2)
...om/linecorp/armeria/client/HttpSessionHandler.java 57% <0%> (-1.87%) 24% <0%> (-1%)
.../linecorp/armeria/client/Http1ResponseDecoder.java 60% <0%> (-1.82%) 23% <0%> (-1%)
...inecorp/armeria/client/grpc/ArmeriaClientCall.java 80.15% <0%> (-1.53%) 36% <0%> (-1%)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff6ca72...0221896. Read the comment docs.

@trustin trustin merged commit 2cb3241 into line:master Dec 11, 2018
@trustin trustin deleted the cleaner_decorator_api branch December 11, 2018 03:15
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:

Practically, there are only 2 ways of adding a client decorator:

- `builder.decorator(HttpRequest.class, HttpResposne.class, ...);`
- `builder.decorator(RpcRequest.class, RpcResponse.class, ...);`

Instead of requiring a user to specify req/res types, we could just
provide two builder methods dedicated to each case.

Modifications:

- Deprecated the old `decorator()` builder methods.
- Added `decorator(Function)` and `decorator(ClientDecoratorFunction)`
  which adds an HTTP-level decorator.
- Added `rpcDecorator(Function)` and `rpcDecorator(ClientDecoratorFunction)`
  which adds an RPC-level decorator.
- Removed `HttpClient.decorator()` methods which became unnecessary due
  to the new `decorator()` methods in `AbstractClientOptionsBuilder`.

Result:

- Fixes line#1482
- Simpler builder API
- Leaner user code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants