brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ahgittin <>
Subject [GitHub] incubator-brooklyn pull request: Adds boolean ConfigKey.isNonNull(...
Date Wed, 26 Nov 2014 01:05:46 GMT
Github user ahgittin commented on the pull request:
    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 or file a JIRA ticket
with INFRA.

View raw message