Convert AttributesExtractor to interface #4363
Conversation
iNikem
requested review from
anuraaga,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| @@ -45,8 +45,7 @@ protected abstract void onEnd( | |||
| * Sets the {@code value} with the given {@code key} to the {@link AttributesBuilder} if {@code | |||
| * value} is not {@code null}. | |||
| */ | |||
| protected static <T> void set( | |||
| AttributesBuilder attributes, AttributeKey<T> key, @Nullable T value) { | |||
| default <T> void set(AttributesBuilder attributes, AttributeKey<T> key, @Nullable T value) { | |||
There was a problem hiding this comment.
Since set isn't really part of the contract of AttributesExtractor, we should keep them off the method even as defaults. I'm thinking the use case of "fill attribute if available" is so common that it's fine to add the @Nullable annotation in the API and consider it "correct usage". So easiest is probably to rely on the implementation detail that put can be called directly, and I can work on that in the SDK to formalize it.
Can be another PR but we should do it
There was a problem hiding this comment.
@jkwatson explicitly told me on Slack not to rely on null-checking of the put :) But if that gets changed in SDK/API, I will be happy to remove this method.
There was a problem hiding this comment.
I'd say either rely on it, or move to an internal class - I guess one of the motivations of the change is to make this interface stable :) I don't think the set method can be part of that.
Fixes #3131