-
Notifications
You must be signed in to change notification settings - Fork 299
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
Make @ChecksForNull
an alias for @Nullable
#397
Conversation
The `@ChecksForNull` annotation is used by SpotBugs (and FindBugs) and has the same semantic as `@Nullable` for NullAway. The `@Nullable` annotation is ignored by Spotbugs. For details refer to https://spotbugs.readthedocs.io/en/stable/annotations.html.
public void checkForNullSupport() { | ||
compilationHelper | ||
.addSourceLines( | ||
"Nullable.java", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this new test for the @Nullable
annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to show that the same outputs are created for both SpotBugs annotations. But I can remove this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment to that effect (e.g. // This is just to check the behavior is the same between @Nullable and CheckForNull
) then change the file name to TestNullable.java
(it's not used elsewhere in the tests, but ideally classes should be in a file with the same name as the class and this is used as the file name that javac
sees when compiling the test case, I believe 🙂 ).
Other than that, it looks good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Couple nits / questions on the test
" public void run() {System.out.println(nullable.toString());}", | ||
"}") | ||
.addSourceLines( | ||
"CheckForNull.java", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be TestCheckForNull.java
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll fix that. Is this value actually used somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to land! 🎉
I see everything, including CLA done, so I am merging, landing, and closing the original issue. Thanks for the PR @uhafner ! |
`CheckForNull` is now supported by NullAway as well: See uber/NullAway#397
`CheckForNull` is now supported by NullAway as well: See uber/NullAway#397
`CheckForNull` is now supported by NullAway as well: See uber/NullAway#397
`CheckForNull` is now supported by NullAway as well: See uber/NullAway#397
The
@ChecksForNull
annotation is used by SpotBugs (and FindBugs) and has the same semantic as@Nullable
for NullAway. The@Nullable
annotation is ignored by Spotbugs so projects can't use SpotBugs and NullAway together. For details refer to the SpotBugs documentation.This PR fixes #99.
Thank you for contributing to NullAway!
Please note that once you click "Create Pull Request" you will be asked to sign our Uber Contributor License Agreement via CLA assistant.
Before pressing the "Create Pull Request" button, please provide the following:
A description about what and why you are contributing, even if it's trivial.
The issue number(s) or PR number(s) in the description if you are contributing in response to those.
If applicable, unit tests.