-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
Fix issue S1130 Exceptions in 'throws' clauses should not be superfluous #9073
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.
I don't think that we want to remove the declared exceptions from the method declarations.
|
||
{ |
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 don't understand this change. The method body includes a throw new NoSuchElementException();
and yet the method declaration has removed the throws NoSuchElementException
.
I also don't understand why the indentation was changed on line 286 and the blank line was inserted. Those both seem like unnecessary changes.
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.
Hi @MarkEWaite NoSuchElementException does not need to be propagated because it inherits from the RuntimeException class. Not propagating exceptions that are propagated automatically simplifies reading the code. This is the purpose of this Sonar rule.
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.
There is indeed an error in the formatting of the code, certainly caused by the deletion of the throws statement. Unfortunately, these changes were made automatically by our software. There is no other way than to correct this formatting error manually. It would be a shame not to accept this PR correcting 60 issues for this type of error. What do you think?
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.
We're still going to analyse this formatting issue because it's not the expected behaviour.
Correcting code formatting
@@ -158,7 +158,7 @@ public void addListener(@NonNull ExtensionListListener listener) { | |||
* | |||
* Meant to simplify call inside @Extension annotated class to retrieve their own instance. | |||
*/ | |||
public @NonNull <U extends T> U getInstance(@NonNull Class<U> type) throws IllegalStateException { | |||
public @NonNull <U extends T> U getInstance(@NonNull Class<U> type) { |
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.
Effective Java 3rd Edition §74 says:
Use the Javadoc
@throws
tag to document each exception that a method can throw, but do not use thethrows
keyword on unchecked exceptions.
This PR removes the throws
keyword on this unchecked exception (good), but it does not add the Javadoc @throws
tag (bad).
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 dilemma is either to keep the existing code which returns unchecked exceptions or to validate the PR which fixes this case but does not declare the exception unchecked. Another solution would be to make these corrections manually. According to Sonar's estimate, it would take around 5 hours of work to remediate the 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.
It's not a dilemma for me, as I simply won't approve this PR unless the changes I requested are made. According to my estimate, it would take around 15 minutes of work to make the requested changes.
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.
It was just a suggestion for improvement that you are free to reject. Would you like me to close the PR?
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.
It's not a net improvement if it trades one problem for another. I see no reason for this PR to be closed once the requested changes are made.
@@ -612,7 +612,7 @@ public static String getCookie(HttpServletRequest req, String name, String defau | |||
private static final Pattern ICON_SIZE = Pattern.compile("\\d+x\\d+"); | |||
|
|||
@Restricted(NoExternalUse.class) | |||
public static String validateIconSize(String iconSize) throws SecurityException { | |||
public static String validateIconSize(String iconSize) { |
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.
Effective Java 3rd Edition §74 says:
Use the Javadoc
@throws
tag to document each exception that a method can throw, but do not use thethrows
keyword on unchecked exceptions.
This PR removes the throws
keyword on this unchecked exception (good), but it does not add the Javadoc @throws
tag (bad).
@@ -832,7 +832,7 @@ public static Jenkins get() throws IllegalStateException { | |||
*/ | |||
@Deprecated | |||
@NonNull | |||
public static Jenkins getActiveInstance() throws IllegalStateException { | |||
public static Jenkins getActiveInstance() { |
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.
Effective Java 3rd Edition §74 says:
Use the Javadoc
@throws
tag to document each exception that a method can throw, but do not use thethrows
keyword on unchecked exceptions.
This PR removes the throws
keyword on this unchecked exception (good), but it does not add the Javadoc @throws
tag (bad).
@@ -592,7 +592,7 @@ public synchronized int maxNumberOnDisk() { | |||
return numberOnDisk.max(); | |||
} | |||
|
|||
protected final synchronized void proposeNewNumber(int number) throws IllegalStateException { | |||
protected final synchronized void proposeNewNumber(int number) { |
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.
Effective Java 3rd Edition §74 says:
Use the Javadoc
@throws
tag to document each exception that a method can throw, but do not use thethrows
keyword on unchecked exceptions.
This PR removes the throws
keyword on this unchecked exception (good), but it does not add the Javadoc @throws
tag (bad).
@@ -100,7 +100,7 @@ public void forceRemoveRecursive(@NonNull Path path) throws IOException { | |||
@Restricted(NoExternalUse.class) | |||
@FunctionalInterface | |||
public interface PathChecker { | |||
void check(@NonNull Path path) throws SecurityException; | |||
void check(@NonNull Path path); |
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.
Effective Java 3rd Edition §74 says:
Use the Javadoc
@throws
tag to document each exception that a method can throw, but do not use thethrows
keyword on unchecked exceptions.
This PR removes the throws
keyword on this unchecked exception (good), but it does not add the Javadoc @throws
tag (bad).
@@ -49,7 +49,7 @@ public interface Authentication extends Principal, Serializable { | |||
|
|||
boolean isAuthenticated(); | |||
|
|||
void setAuthenticated(boolean isAuthenticated) throws IllegalArgumentException; | |||
void setAuthenticated(boolean isAuthenticated); |
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.
Effective Java 3rd Edition §74 says:
Use the Javadoc
@throws
tag to document each exception that a method can throw, but do not use thethrows
keyword on unchecked exceptions.
This PR removes the throws
keyword on this unchecked exception (good), but it does not add the Javadoc @throws
tag (bad).
@@ -38,7 +38,7 @@ public boolean hasMoreElements() { | |||
} | |||
|
|||
@Override | |||
public T nextElement() throws NoSuchElementException { | |||
public T nextElement() { |
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.
Creates an unused import for NoSuchElementException
.
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.
Thank you for identifying this problem, we'll be looking very quickly at how to ensure that 'Indepth' doesn't reproduce this issue again.
Remove the unnecessary import of NoSuchElementException
The aim of this RP is to remove violations of the Sonar rule S1130 Exceptions in 'throws' clauses should not be superfluous.
Superfluous exceptions within throws clauses have negative effects on the readability and maintainability of the code. An exception in a throws clause is superfluous if it is:
These changes have been made automatically by our "Indepth" java code remediation solution, which aims to improve code quality. You can also use it periodically to remove issues that have appeared in the source code. Use of this solution is free for all open source projects. You can find all the documentation and the download link on the site https://www.indepth.fr/