-
Notifications
You must be signed in to change notification settings - Fork 96
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
[MJAVADOC-825] Prefer NullPointerExceptions for null arguments #350
Conversation
@@ -534,11 +534,16 @@ protected static JavaVersion getJavadocVersion(File javadocExe) | |||
* @return the version of the javadoc for the output, only digits and dots | |||
* @throws PatternSyntaxException if the output doesn't match with the output pattern | |||
* {@code (?s).*?[^a-zA-Z]([0-9]+\\.?[0-9]*)(\\.([0-9]+))?.*}. | |||
* @throws IllegalArgumentException if the output is null | |||
* @throws NullPointerException if the output is null | |||
* @throws IllegalArgumentException if the output is empty | |||
*/ | |||
protected static String extractJavadocVersion(String output) throws IllegalArgumentException { |
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.
Given IAEx
being declared as thrown by, perhaps NPEx
could also be added for style consistency?
Or IAEx
declaration removed?
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.
Both have @throws clauses. Or did you mean something else?
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.
IllegalArgumentException
is RTEx
(as NPEx
is). And is declared as being thrown by this method. Thus - let's add NullPointerException
to the throws
list.
Or remove throws
.
Here, and in the second modified method.
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.
But that's already done in this PR. Are you looking at the left side (old code) instead of the right side (new code)?
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 see this:
protected static String extractJavadocVersion(String output) throws IllegalArgumentException {
I'd like to see this instead:
protected static String extractJavadocVersion(String output) throws IllegalArgumentException, NullPointerException {
or this:
protected static String extractJavadocVersion(String output) {
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.
Good spot. That should go the other way per guidelines. Removed both.
No description provided.