Some sonar fixes #1141
Some sonar fixes #1141
Conversation
| @@ -34,7 +34,7 @@ | |||
| map.put(b, a); | |||
| } | |||
|
|
|||
| public static ArrayList<ParameterSignature> signatures(Method method) { | |||
| public static List<ParameterSignature> signatures(Method method) { | |||
There was a problem hiding this comment.
This change could break people.
There was a problem hiding this comment.
Is there way to migrate to List without breaking people?
There was a problem hiding this comment.
Not in public API signatures. If we have a public method that returns an ArrayList we can't change it to return a List:
ArrayList<String> s = ParameterSignature.signatures(method); // woops!
There was a problem hiding this comment.
I understand. I ask about some best practices like a "add deprecation with a comment about migration to List in next releases". Btw I put it back to the ArrayList
There was a problem hiding this comment.
I personally don't think it's worth it to deprecate the method and replace it with a similarly-named method that returns a List
|
Thanks for the fixes! The 'static public' bothers me too :-) |
| @@ -34,6 +34,15 @@ | |||
| * @since 4.4 | |||
| */ | |||
| public class Assume { | |||
|
|
|||
| /** | |||
| * Do not use instance if this class. | |||
There was a problem hiding this comment.
Let's change this Javadoc to simply "Do not instantiate." Ditto for ResultMatchers and Classes
|
LGTM. I can fix the minor problems that are left when I merge this. @junit-team/junit-committers can one of you please take a look? |
|
@kcooney done |
| } | ||
| } | ||
|
|
||
| static private boolean doubleIsDifferent(double d1, double d2, double delta) { |
There was a problem hiding this comment.
These implementations (floatIsDifferent, doubleIsDifferent) were deliberate stylistic choices for readability. I don't feel strongly about the style, but feel that muddying the git history for a different stylistic change doesn't seem worth it.
|
Mostly great stuff. I've commented the few places that give me pause: in places where there's at best a minor improvement in style, or just a fit to a different preference, I'd prefer not to make the change, a lesson learned since our "git blame" history became largely useless after we "fixed" all the indentation. |
|
@dsaff thx for the review. All the comments are fixed now. |
|
LGTM! @dsaff Do you want to take another look? |
|
LGTM2! |
No description provided.