Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/356#issuecomment-64499719
The code is good but there I'm not clear what problem this solves. We are
adding an API method but it's not simplifying the code anywhere. It has the
potential to do so I think but I'd like to see the use case elaborated.
The reason for this is that it feels like we're solving a slightly
different problem to the one we want to. What we are currently doing is
forbidding a user to set a `null`. I don't know of any places where we
accidentally call `set(null)` so I don't think this is quite what we want to do.
I think what we want to do instead is to guarantee that a `get()` on a key
will never return null. Currently however if there is no default value, we
still might get null (if the key is never set). So I think `nonNull()` is
useful mainly when used with `defaultValue(nonNullThing)`.
Also, in some places we set `null` to mean "don't inherit your parent's
value". And that still might be useful. (But currently that only has the
desired effect if the default value is null, or if the caller does
`get(KEY)!=null ? get(KEY) : KEY.defaultValue()` ... and I'm not sure if that
latter one is used in many places.)
So maybe what we want to do, instead of disallowing `set(null)`, would be
to when we `get()` the value if the value is explicitly null and the key is
`nonNull()`, then we return the default value. In other words, instead of
`nonNull()` it could be called `useDefaultValueIfNull()` ? But I still wonder
is that useful enough?
Are there use cases I'm not thinking of?
Also, if we do want to merge this, should we mark `nonNull()` and
`isNonNull()` as `@Beta` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
