Some sonar fixes #1141
Some sonar fixes #1141
Conversation
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 :-) |
src/main/java/org/junit/Assume.java
Outdated
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 |
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! |
4 participants
No description provided.