ThreeTen / threeten-extra

Provides additional date-time classes that complement those in JDK 8
http://www.threeten.org/threeten-extra/
BSD 3-Clause "New" or "Revised" License
396 stars 77 forks source link

NotNull annotations #170

Open spand opened 3 years ago

spand commented 3 years ago

I searched the issues but seems uncovered. Has it been considered to add NotNull annotations to various methods, parameters and such?

We use the library from Kotlin and having nullability explicit in the api would be really nice.

In any case, thanks for the awesome library!

jodastephen commented 3 years ago

I'm not a fan of those annotations myself. IMO they are not checked without tooling and thus they provide false reassurance.

I'm reluctant to offer to accept a PR, as that would mean I would have to maintain the annotations going forward, and would have to ensure they are correct on all new additions.

spand commented 3 years ago

I mostly see them as documentation as code. As you maybe allude to they can be quite disruptive if actually incorrect (nullable marked as NotNull, opposite seems harmless) but coming from Kotlin I see the benefits of explicit nullability every day and is obviously quite a fan of it.

The problem is not big though as you have been quite good at keeping null values out of the API but I would argue that the documentation is sort of missing in the places you would add the annotation. The example that triggered me to actually discover the annotations weren't present as I had assumed was methods like Interval.getStart(), Interval.getEnd(). You could argue the it follows from the factory methods but having the info when you need it is preferable since with Java apis you just never know.

I understand your concern as the maintainer. Maybe others can provide more persuasive argument in the future.

Kind regards

jodastephen commented 3 years ago

Feel free to raise a PR to add ", can return null" or ", not null" to the end of Javadoc @return if that helps you

sdavids commented 5 months ago

I suggest to use jspecify once it is 1.0.