-
Notifications
You must be signed in to change notification settings - Fork 294
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
Avoid suggesting castToNonNull fixes in certain cases #799
Conversation
Pull Request Test Coverage Report for Build #1182
💛 - Coveralls |
Tested this more and didn't see any weird crashes or unexpected behaviors, so I think it's ready for review. |
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.
Overall looks good, minor comments below.
" return castToNonNull(o);", | ||
" }", | ||
"}") | ||
.doTest(); | ||
} | ||
|
||
@Test | ||
public void suppressInsteadOfCastToNonNull() throws IOException { |
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.
Minor comment noting which cases these are and referencing the ErrorBuilder docs? Just because when not seen as part of the PR it might be hard to remember why we want this behavior.
Also, can we add a case where null
is assigned to a non-null field?
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.
Fixed in b84a4f7
@lazaroclapp addressed your comments and also added a few more tests for |
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 great to me! Appreciate the added coverage of existing code.
What I believe to be the most minor nit humanly possible below:
Co-authored-by: Lázaro Clapp <[email protected]>
We avoid suggesting a
castToNonNull
fix if either (1) the expression is thenull
literal (i.e., don't suggestcastToNonNull(null)
), or (2) the expression is a@Nullable
parameter (see code comments for justification). In these cases, we suggest a@SuppressWarnings
annotation instead.Here we also add some tests to improve coverage of the
ErrorBuilder
class.