Fixes false negatives in UnnecessaryAbstractClass#4399
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4399 +/- ##
============================================
+ Coverage 84.33% 84.35% +0.01%
- Complexity 3272 3281 +9
============================================
Files 473 472 -1
Lines 10351 10356 +5
Branches 1826 1828 +2
============================================
+ Hits 8730 8736 +6
Misses 668 668
+ Partials 953 952 -1
Continue to review full report at Codecov.
|
a386e35 to
9a646f2
Compare
schalkms
left a comment
There was a problem hiding this comment.
I guess the mentioned code snippets aren't flagged a second time by the empty rule set, right?
...t-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/baseline/IndentingXMLStreamWriter.kt
Show resolved
Hide resolved
...es-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryAbstractClass.kt
Outdated
Show resolved
Hide resolved
| NamedClassMembers(klass, namedMembers) | ||
| .detectAbstractAndConcreteType() | ||
| } else if (!klass.hasConstructorParameter()) { | ||
| report(CodeSmell(issue, Entity.from(klass), noConcreteMember), klass) |
There was a problem hiding this comment.
Where did you improve the message description?
This PR also improves the messages reported for this rule.
There was a problem hiding this comment.
You are right. I didn't "improve the message". Before there were some issues that had the message "An abstract class without an abstract member can be refactored to a concrete class." but it should be "An abstract class without a concrete member can be refactored to an interface." and the improvement was that. I set the correct message to some issues that were getting the wrong one.
...tyle/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryAbstractClassSpec.kt
Outdated
Show resolved
Hide resolved
...tyle/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnnecessaryAbstractClassSpec.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com>
I guess that some cases are flagged by that rule set too. And I think that it's good that both rules flag it because one developer could have one of those issues disabled. |
|
If I remember correctly, we excluded those cases for exactly this reason. Thus, the same code is not flagged twice by two different rules. I could be wrong here, however.
|
|
I didn't introduce those cases in this PR. Those cases where there already, they weren't tested but they were flagged. I just extended the tests to ensure that I didn't break anything. |
|
I'm afraid, I didn't understand this statement. As I understood the description of the rule, you fixed a false-positive in this PR (e.g.
|
|
This rule was already flagging code at the same time as the empty rule set. I didn't introduce that issue here. For example: abstract class A : B {}It was flagged by For that reason I say that I didn't introduce this "double report", it was there. For sure, now there are more "double reports" but that is because now |
|
Thanks for the elaboration, Brais! It's good to merge. |
This PR fixes a false positive in
UnnecessaryAbstractClass.Before this PR the class
Bwasn't flagged:And the same here:
This PR also improves the messages reported for this rule.