-
-
Notifications
You must be signed in to change notification settings - Fork 759
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
UnusedPrivateProperty improvements and refactoring out new UnusedVariable rule #7089
UnusedPrivateProperty improvements and refactoring out new UnusedVariable rule #7089
Conversation
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
This is more or less done, but this breaks the unused variable checks (i.e. failng tests). The previous version of |
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
fd6e6d9
to
ad20020
Compare
I'm ok about splitting this rule in two. And there is other thing here, we are moving another rule away from plain detekt. I'm ok with that in general. Correctness is better than speed. But I wanted just to make that clear for anyone looking at this PR. |
@@ -56,103 +67,163 @@ class UnusedPrivateProperty(config: Config) : Rule( | |||
setOf("UNUSED_VARIABLE", "UNUSED_PARAMETER", "unused", "UnusedPrivateMember") | |||
|
|||
@Configuration("unused property names matching this regex are ignored") | |||
private val allowedNames: Regex by config("_|ignored|expected|serialVersionUID", String::toRegex) | |||
private val allowedNames: Regex by config( | |||
"_|ignored|expected|serialVersionUID", |
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.
If this rule only works with properties then _
should be removed.
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.
sure
@BraisGabin how should I go with the unused variable rule, I can add it to this PR or do that in a new PR (we can create a new 3rd PR as a base for both). Also, what should be the name for the new rule |
I'm also fine with it 👍
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7089 +/- ##
============================================
+ Coverage 84.67% 84.71% +0.04%
- Complexity 3970 3975 +5
============================================
Files 575 576 +1
Lines 12051 12139 +88
Branches 2476 2497 +21
============================================
+ Hits 10204 10284 +80
Misses 621 621
- Partials 1226 1234 +8 ☔ View full report in Codecov by Sentry. |
|
detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariable.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
Fixed
Show fixed
Hide fixed
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Fixed
Show fixed
Hide fixed
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Fixed
Show fixed
Hide fixed
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Fixed
Show fixed
Hide fixed
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Fixed
Show fixed
Hide fixed
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Fixed
Show fixed
Hide fixed
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Fixed
Show fixed
Hide fixed
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Fixed
Show fixed
Hide fixed
7cb02ac
to
42b5e78
Compare
42b5e78
to
8e3c445
Compare
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.
Top level properties shouldn't be handled by UnusedVariables
because they aren't variables, they are properties. That should remain on UnusedPrivateProperty
.
Other than that the PR looks really good!
Off-topic: Do we have a rule to check that all the parameters of a lambda are used? 🤔 I thought we have but I can't find it.
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Outdated
Show resolved
Hide resolved
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Outdated
Show resolved
Hide resolved
...kt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariableSpec.kt
Outdated
Show resolved
Hide resolved
...kt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariableSpec.kt
Outdated
Show resolved
Hide resolved
...kt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariableSpec.kt
Outdated
Show resolved
Hide resolved
...kt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariableSpec.kt
Outdated
Show resolved
Hide resolved
...kt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariableSpec.kt
Outdated
Show resolved
Hide resolved
...kt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariableSpec.kt
Outdated
Show resolved
Hide resolved
...kt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariableSpec.kt
Outdated
Show resolved
Hide resolved
detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariable.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Brais Gabín <braisgabin@gmail.com>
…ate-property' into false-negative-unused-private-property
Co-authored-by: Brais Gabín <braisgabin@gmail.com>
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.
Add one test more and this is good to go for me :) really good job! Thank you!
...s-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivatePropertySpec.kt
Outdated
Show resolved
Hide resolved
detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariable.kt
Outdated
Show resolved
Hide resolved
...kt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedVariableSpec.kt
Show resolved
Hide resolved
Co-authored-by: Brais Gabín <braisgabin@gmail.com>
@psuzn can you rebase/merge your pr with main? It has conflicts. After that this is ready to merge. |
…-private-property
should be good to go. |
Fixes #6702 and refactors the unused variable rules into its own separate
UnusedVariable
rule.