search

migrator / guava-libraries-3

Guava: Google Core Libraries for Java 1.6+
0 stars 0 forks source link

Improve Joiner#join documentation and exception message for null behavior #48

Open migrator opened 9 years ago

migrator commented 9 years ago

I'm not sure whether it is an actual bug so I formulated the subject as a question, but I think that if some instances of Joiner (namely those not generated from skipNulls() or useForNull(String)) don't support null arguments and explicitly throw an NPE the parameters of the Joiner#join() method shouldn't be annotated @Nullable.

I also think having a subtype of Joiner (something like NullSupportedJoiner) just for this wouldn't make sense, so if this is done to prevent unnecessary static analysis warnings on Joiner#join() method calls with possibly null parameter values, I would suggest that it at least is added to the documentation of the join() method (that the Joiner instance needs to support null values for the parameters to be nullable). It IS explained in the javadoc of the class but it's easy to miss when just looking at the method documentation (e.g. from an IDE).

Throwing a more explicit error (e.g. adding a message about "skipNulls()" call requirement to the thrown NPE) may also be a nice way to document this requirement.

Let me know if I misunderstood something.

relevance: 2

migrator commented 9 years ago

summary: Not Defined

I think we take the position that a parameter has to be @Nullable if any implementation of the type permits nulls there. This is certainly not ideal. (It's also not ideal that I'm not 100% sure that I have our policy right.) For general despair around the issue of nullability annotations, see issue 1812.

For this specific case, we can be more optimistic. We can improve the error message and the doc. I'm modifying this bug's title to focus on that.

status Not Defined creator: cpov...@google.com created at: Sep 18, 2014