Add a simpler way to decorate a client / Enrich ClientBuilder #93
Conversation
trustin
force-pushed
the
more_client_builder_methods
branch
from
5cdf861
to
4971d56
Compare
| * {@link Client} with the specified {@code handlerDecorator}. | ||
| */ | ||
| default <T extends RemoteInvoker, U extends RemoteInvoker> | ||
| Client decorateInvoker(Function<T, U> handlerDecorator) { |
There was a problem hiding this comment.
I think invokerDecorator is better than handlerDecorator here
There was a problem hiding this comment.
Ugh, copy & paste cruft! Fixed. :-)
trustin
force-pushed
the
more_client_builder_methods
branch
from
4971d56
to
3348404
Compare
| @SuppressWarnings("unchecked") | ||
| final Client newClient = decorator.apply(this); | ||
|
|
||
| if (newClient != null) { |
There was a problem hiding this comment.
Why would it be null? If a decorator wanted to not decorate for some reason, it could just return the input client.
There was a problem hiding this comment.
Just wanted to yield the same behavior with the server side, but yeah, we shouldn't do this on both sides. Let me fix this.
There was a problem hiding this comment.
I thin we still need to check if a decorator returns null to generate more meaningful message (e.g. you screwed up, not Armeria did)
|
@anuraaga Addressed the comments. Anything else? |
| default Client decorate(Function<Client, Client> decorator) { | ||
| @SuppressWarnings("unchecked") | ||
| final Client newClient = decorator.apply(this); | ||
| if (newClient == null) { |
|
LGTM - guess we only use requireNonNull on parameters, but just checking |
|
LGTM |
|
@anuraaga Ah, I should use it for checking return values. |
|
@anuraaga But we are using dynamically generated exception messages in this case, so I'd like to leave them as they are. |
|
@synk Please merge when you're ready. :-) I did not squash the commits because they are different. |
trustin
force-pushed
the
more_client_builder_methods
branch
from
05d5f42
to
9abee34
Compare
Motivation:
When a user attempts to decorate a client, it is much more verbose than
decorating a service. A user should be given with a simpler way to
configure a client when using ClientBuilder.
Modifications:
- Add a new interface 'Client' which is a client-side counterpart of
'Service'
- Add SimpleClient and DecoratingClient
- Add various setter methods incloding decorator() to ClientBuilder
- Add LoggingClient and hide LoggingClientCodec
- Remove the ability to decorate an InvocationHandler, because it
seems that RemoteInvoker decorators can do the exact same job.
- Miscellaneous:
- Make LoggingClientCodec extend DecoratingClientCodec
- Make AbstractOptions and its subtypes accept
Iterable<AbstractOption> as well
Result:
A user can build a client in a more fluent and less verbose way:
builder.decorator(LoggingClient::new)
.decorator(MyDecorator::new)
.build();
- Disallow null in AbstractOptions for clarity - Disallow returning null in decorator.apply() for clarity
Add a simpler way to decorate a client / Enrich ClientBuilder
|
Merged! |
|
@synk Thanks! :-) |
Motivation:
When a user attempts to decorate a client, it is much more verbose than
decorating a service. A user should be given with a simpler way to
configure a client when using ClientBuilder.
Modifications:
'Service'
seems that RemoteInvoker decorators can do the exact same job.
Iterable as well
Result:
A user can build a client in a more fluent and less verbose way: