-
Notifications
You must be signed in to change notification settings - Fork 769
Update UnsafeLocaleUsage
with detection for Locale.of
#5279
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
base: master
Are you sure you want to change the base?
Conversation
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.
Just something I noticed on a quick skim.
Co-authored-by: Stephan Schroevers <[email protected]>
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.
Thanks for this!
+ " (which takes in an IETF BCP 47-formatted string) or a Locale.Builder" | ||
+ " (which throws exceptions when the input is not well-formed).\n" | ||
+ "Please read the Error Prone documentation page for UnsafeLocaleUsage for" | ||
+ " more information."); |
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.
The generated diagnostics automatically include the link to the full 'explanation', so writing this out here isn't necessary
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.
Done.
Description.Builder descriptionBuilder = | ||
buildDescription(tree) | ||
.setMessage( | ||
"Avoid using Locale.of static methods (which do not check their arguments 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.
By default, the message is the BugPattern.summary
. Is there information here that applies to all findings that could go in the summary instead?
If you want to customize this to mention e.g. Locale.of
vs. Locale constructors
, WDYT about refactoring and using a format string to share the text that's the same for both?
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.
Between constructors and Locale.of
there is a lot of sharing (they are basically the same).
I refactored the messages to share, as suggested.
toString
throws a wrench in reusing much in the summary.
But while reviewing it I discovered that java.util.Locale
is not a library :-), and I fixed that.
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.
Done
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.
Done, thank you.
Description.Builder descriptionBuilder = | ||
buildDescription(tree) | ||
.setMessage( | ||
"Avoid using Locale.of static methods (which do not check their arguments 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.
Between constructors and Locale.of
there is a lot of sharing (they are basically the same).
I refactored the messages to share, as suggested.
toString
throws a wrench in reusing much in the summary.
But while reviewing it I discovered that java.util.Locale
is not a library :-), and I fixed that.
+ " (which takes in an IETF BCP 47-formatted string) or a Locale.Builder" | ||
+ " (which throws exceptions when the input is not well-formed).\n" | ||
+ "Please read the Error Prone documentation page for UnsafeLocaleUsage for" | ||
+ " more information."); |
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.
Done.
Fix for #5278