Skip to content
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

Fixes false negatives in UnnecessaryAbstractClass #4399

Merged
merged 5 commits into from Jan 2, 2022

Conversation

BraisGabin
Copy link
Member

This PR fixes a false positive in UnnecessaryAbstractClass.

Before this PR the class B wasn't flagged:

interface A {
    val i: Int
}
abstract class B : A

And the same here:

abstract class A {
    abstract val i: Int
}
abstract class B : A()

This PR also improves the messages reported for this rule.

@codecov
Copy link

codecov bot commented Dec 26, 2021

Codecov Report

Merging #4399 (cfcaf6d) into main (70e05ac) will increase coverage by 0.01%.
The diff coverage is 56.00%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
...h/detekt/core/baseline/IndentingXMLStreamWriter.kt 56.33% <45.00%> (+0.62%) ⬆️
...sch/detekt/rules/style/UnnecessaryAbstractClass.kt 86.66% <100.00%> (-0.29%) ⬇️
...detekt/rules/style/UnderscoresInNumericLiterals.kt 87.23% <0.00%> (+4.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70e05ac...cfcaf6d. Read the comment docs.

@BraisGabin BraisGabin force-pushed the improve-unnecessary-abstract-class branch from a386e35 to 9a646f2 Compare December 26, 2021 19:16
@BraisGabin BraisGabin added this to the 1.20.0 milestone Dec 26, 2021
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the mentioned code snippets aren't flagged a second time by the empty rule set, right?

NamedClassMembers(klass, namedMembers)
.detectAbstractAndConcreteType()
} else if (!klass.hasConstructorParameter()) {
report(CodeSmell(issue, Entity.from(klass), noConcreteMember), klass)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did you improve the message description?

This PR also improves the messages reported for this rule.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com>
@BraisGabin
Copy link
Member Author

I guess the mentioned code snippets aren't flagged a second time by the empty rule set, right?

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.

@schalkms
Copy link
Member

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 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.

@BraisGabin
Copy link
Member Author

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.

@schalkms
Copy link
Member

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. abstract class B : A as well as abstract class B : A()).

I didn't introduce those cases in this PR. Those cases where there already, they weren't tested but they were flagged.

@BraisGabin
Copy link
Member Author

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 UnnecessaryAbstractClass and by EmptyClassBlock.

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 UnnecessaryAbstractClass is working as expected.

@schalkms
Copy link
Member

Thanks for the elaboration, Brais! It's good to merge.

@BraisGabin BraisGabin merged commit eb0f1cf into main Jan 2, 2022
@BraisGabin BraisGabin deleted the improve-unnecessary-abstract-class branch January 2, 2022 17:58
@cortinico cortinico added the rules label Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants