Some file module Sonar fixes #2718
Conversation
|
There are travis failures. |
garyrussell
left a comment
garyrussell
left a comment
There was a problem hiding this comment.
Good work! - just a few minor comments.
|
|
||
| public AbstractRegexPatternFileListFilter(String pattern) { | ||
| this.pattern = Pattern.compile(pattern); | ||
| this(Pattern.compile(pattern)); |
| * @since 5.1.3 | ||
| */ | ||
| public void setAge(Duration age) { | ||
| setAge(age.get(ChronoUnit.SECONDS)); |
There was a problem hiding this comment.
Why not just age.getSeconds() ? Simpler, and functionally the same.
There was a problem hiding this comment.
Because it is not public 😄
There was a problem hiding this comment.
toSeconds() is private, but...
public long getSeconds() {
return seconds;
}| return this.name; | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
StreamHolder ctor must be protected to avoid synthetic ctor.
|
I think that's enough for the current round. Another point is to work out a rule for ourselves to check Sonar report when we touch some classes and fix smells together with the original reported issue. I will push fixes according your review when you done. Thanks |
I don't think I want to go to the trouble of a Sonar build for every PR (or even going to check Sonar to see if changed files have existing smells); right now, I am fine with our practice of fixing any new smells we introduce (or are reported because we touched a file) the next day when we get an alert. As long as we obey the "no new smells" rule and "fewer smells in this release than the last one" rule, we will eventually reduce them to zero. We have removed ~1000 smells (45%) in just a short few months (and mainly done that as background tasks). |
2 participants
No description provided.