Open electrum opened 1 year ago
Just my 2 cents, as a maintainer: although the behavior is documented as "undefined", currently we drop any null-valued attributes. It's "undefined" because the spec doesn't specify what the behavior is supposed to be. However, we are very unlikely to change this behavior in the foreseeable future, as it would be a major change in the behavior of the system (and one that we don't think would provide any value to the end-user).
So, I think I would not be in favor of adding Optional
parameters at this point.
My $0.02 -- I could see at first glance why one might want this, but I've been frowning at Optional
method arguments for so long it might require some additional convincing. I say just pass the null.
Since this is really tackling setAttribute(AttributeKey<T>, T)
would consideration also be given to setAttribute(String, Optional<String>)
? That paves the way for setAttribute(String, Optional<Long>)
and all the other types...which seems like it would just create a mess.
I think the apply(consumer)
reads considerably worse than the other approach, and it feels forced -- I'd say just break the chain.
Related to #4336.
Thanks for the link to #4336. Changing the specification to ignore nulls, which matches the current behavior of the primary implementation, seems like a better option.
Thanks for the link to #4336. Changing the specification to ignore nulls, which matches the current behavior of the primary implementation, seems like a better option.
Yeah, but keep in mind that some people might rely on this behavior in other languages, further complicating things. Because the behavior is undefined, some languages might clear the value (see #992 and #971 for historical context).
@electrum if you want to open a spec issue advocating for a change from "undefined" to "ignored leaving existing value" I would +1 that.
These types of chains seem common:
Suppose that
route
might be null. The documentation forsetAttribute()
says that "the behavior of null values is undefined", so there's no way to do this without breaking the chain and assigning theSpanBuilder
to a variable.We could add a
setAttribute(AttributeKey<T> key, Optional<T> value)
method:Another possibility is an
apply(Consumer<SpanBuilder> consumer)
method:The
Optional
method is very convenient, especially if you already useOptional
, whereasapply()
is generic and makes it easy to handle anything within the chain, including delegating to a helper method. I'm happy to add both, and add the equivalent methods toAttributesBuilder
.Thoughts?