-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add UserAgents.of(UserAgent, Iterable<Agent>) #902
Conversation
Generate changelog in
|
// Should never hit the following. | ||
throw new SafeIllegalArgumentException( | ||
"Illegal version format. This is a bug", SafeArg.of("version", version())); | ||
} |
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 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.
We should probably update this to use immutables params and avoid the Agent.Builder altogether
.from(base) | ||
.addAllInformational(additional) | ||
.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.
This is a bit odd given other factory methods live in the UserAgent
class, however those don't take a base UserAgent.
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.
good call, not sure why I put it in UserAgents
instead of with other factories in UserAgent
.
I'm not sure if using @Value.Parameter
buys us a ton in terms of either few allocations or readability as we don't currently include Guava in the service-config
module, so no cheap Iterables.concat
:
static UserAgent of(UserAgent base, Iterable<Agent> additional) {
return ImmutableUserAgent.of(
base.nodeId(),
base.primary(),
Stream.concat(base.informational().stream(), StreamSupport.stream(additional.spliterator(), false))
.collect(Collectors.toList()));
}
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.
You're right -- lgtm!
👍 |
Released 2.29.0 |
Before this PR
Creating a
UserAgent
with multiple additional informational agents required several immutable object creations. See palantir/dialogue#1797 for exampleAfter this PR
==COMMIT_MSG==
Add UserAgents.of(UserAgent, Iterable)
==COMMIT_MSG==
Possible downsides?