-
Notifications
You must be signed in to change notification settings - Fork 455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check for nulls eagerly in parameters etc. more consistently #86
Comments
I have tried to contribute to projects before where they were in the middle of a code-style change. I copied the style of the project as it existed, and had to rewrite it to suit the new style they were moving to. Frustrating experience - I got some stuff merged but mostly gave up. The point of spotless is to get rid of these kinds of arguments. In the spirit of spotless, I don't think we should burden our contributors with any stylistic burdens that the tools can't enforce on their own. If you feel strongly about this @jbduncan, you might to look at guava's testlib. Here's a usage example: It calls every public method and confirms that it obeys the @nonnull / @nullable contracts. |
Sounds like a reasonable compromise to me @nedtwigg. I'll look into PackageSanityTests as soon as I've the time. :) |
Hi @nedtwigg, I wonder if I may ask for your help with diagnosing (what I hope is just) a misunderstanding on my part of I've pushed a new branch to https://github.com/diffplug/spotless/tree/issue-86 which has my experiments so far for testing the conformance of Spotless with its @Nonnull/@nullable contracts. However, the So far, I've tried running IntelliJ IDEA's debugger against Do you have the time to help me look into this? If not, then I'd be happy to put this all to one side for someone to tackle again in the future. |
I've got a couple ideas:
|
Hi @nedtwigg, many thanks for the advice. Unfortunately, it seems that no combination of your suggestions seem to work, so I've resorted to using I don't know when I'll be able to send a PR, as I've been focusing on my university studies over the last few weeks, but I suspect that when I am ready I'll need to send multiple sub-PRs, since my unpublished results so far have touched many files in the codebase. If you can think of anything else that I could try, then I would be very happy to listen! Otherwise, I'm more than happy to just continue plugging at this when I have the time to work on it again. |
I don't have any more tricks up my sleeve :) I'm happy to add enhanced null-checking, but I don't want to add a ton of boilerplate test code which makes it harder for people to submit new features in the future. This change is valuable, but minor. If it requires a lot of code, I'm not convinced it's worth the cost. |
Okay, that makes a great deal of sense to me. Since using Thanks @nedtwigg! |
Finished as of #101. |
Basically, use
Objects.requireNonNull
andPreconditions.checkNotNull
more consistently.If we don't want NPE to be thrown when encountering a variable with null, or if null is considered a valid value, then we should annotate it with
@Nullable
.This may get tricky though in cases where we're dealing with e.g. a non-null list containing nullable elements. In that case, we'd need to annotate it like so:
The text was updated successfully, but these errors were encountered: