Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/471#issuecomment-71647237
@michaeldye @sjcorbett The current behaviour (before this PR) is definitely
wrong - of appending `null` to the URL.
Whether or not it should filter nulls automatically is a stylistic
discussion for Brooklyn APIs. Unfortunately we have a bit of a mix - e.g.
`brooklyn.util.collections.MutableList` creation methods take `null` to mean
creating an empty list; other places in our code follows the guava
best-practice of treating `null` as a programming error (failing fast, to catch
these more easily).
In this situation, I lean towards @sjcorbett 's suggestion - a null in the
middle of an array of strings smacks of a programming error, rather than a
short-hand convenience for saying "create an empty thing". The caller can
always filter out the nulls if it truly is valid to have nulls in that context.
Note that passing in an empty string is valid - that will effectively be
ignored.
@michaeldye does that make sense for your use-case, if this were to throw a
`NullPointerException` when the vararg array of `items` contains a `null`?
p.s. in case anyone is interested in the history, one reason we accept
nulls in things like `MutableMap` etc is because of the handling of config and
attributes. The values supplied by the user can include explicit nulls, which
leads to NPEs if you try to just use `ImmutableMap.copyOf(...)`.
---
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.
---
