Clarify that AttributesBuilder.put allows nulls

[tylerbenson]

The underlying implementation already does a null check on both key and value.

I think it's useful to publicly allow nulls on values since they are often null and it's helpful if callers know they don't need to null check.

(I could see an argument to not allow nulls for keys, so I didn't add Nullable there.)

Fixes #4336

[breedx-splk]

I might even think an additional comment is useful -- something which conveys that any existing value for that key will remain if null is passed (a null value doesn't remove/unset any current value).

[tylerbenson]

done.

[breedx-splk]

I like it.

[breedx-splk]

You gotta make the implementations match, though.

[jack-berg]

This would resolve #4336, but see @jkwatson's comment:

Unfortunately, we're currently adhering strictly to the spec:

Attribute values of null are not valid and attempting to set a null value is undefined behavior.

My read of this is that settings a null attribute value is undefined at the spec level, but that doesn't necessarily prevent language implementation from defining the behavior based on what is idiomatic. I suspect that the spec language was coming from this from the perspective of null isn't idiomatic or a consistent concept across languages, and so taking a stance on it at the spec level wouldn't be appropriate.

[breedx-splk]

My read of this is that settings a null attribute value is undefined at the spec level, but that doesn't necessarily prevent language implementation from defining the behavior based on what is idiomatic. I suspect that the spec language was coming from this from the perspective of null isn't idiomatic or a consistent concept across languages, and so taking a stance on it at the spec level wouldn't be appropriate.

That's cool, but that wouldn't prevent/preclude an implementation from clarifying what the behavior is though, would it?

[jack-berg]

That's cool, but that wouldn't prevent/preclude an implementation from clarifying what the behavior is though, would it?

I think its debatable, but my take is that it would not prevent us from clarifying.

[trask]

I don't think we could change the behavior at this point without it being seen as a breaking change, so may as well document it

[jkwatson]

I don't have any strong opposition to this, since it appears the spec will probably never clarify and we might as well just tell people what the current behavior is.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.61%. Comparing base (a01bf6c) to head (ee2a789).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #7271   +/-   ##
=========================================
  Coverage     89.61%   89.61%           
+ Complexity     6863     6859    -4     
=========================================
  Files           780      780           
  Lines         20731    20718   -13     
  Branches       2023     2020    -3     
=========================================
- Hits          18578    18567   -11     
+ Misses         1514     1512    -2     
  Partials        639      639           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.