-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change could break people.
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.
Is there way to migrate to List
without breaking people?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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.