UnusedPrivateMember: detect top level declarations #968
Conversation
|
@schalkms this still doesn't handle detecting private classes, but wanted to get initial feedback before going down that path. It ended up being less complicated than my initial implementation, mainly because all the detection was already in place, and just needed to change the conditions under which they happen (note that there are no new visitors or anything) |
| @@ -103,17 +107,14 @@ class UnusedPrivateMember(config: Config = Config.empty) : Rule(config) { | |||
| } | |||
|
|
|||
| override fun visitProperty(property: KtProperty) { | |||
| if ((property.isPrivate() && property.isNonNestedMember()) | |||
| if ((property.isPrivate() && property.isMemberOrTopLevel()) | |||
There was a problem hiding this comment.
I did some testing around this, and didn't really get why it was trying to exclude nonNestedMembers.
I added some tests for the new functionality.
Note that, before this, something like:
class A {
class B {
private val silentlyIgnored = 1
}
}
wouldn't be detected
There was a problem hiding this comment.
I think @Mauin could help out here.
| @@ -177,7 +177,7 @@ class UnusedPrivateMemberSpec : SubjectSpek<UnusedPrivateMember>({ | |||
| val code = """ | |||
| fun RuleSetProvider.provided() = ruleSetId in defaultRuleSetIds | |||
|
|
|||
| private val defaultRuleSetIds = listOf("comments", "complexity", "empty-blocks", | |||
There was a problem hiding this comment.
had to remove the private, as now the rule would complain about it
| @@ -80,6 +80,6 @@ val stuff = object : Iterator<String?> { | |||
| override fun hasNext(): Boolean = true | |||
| } | |||
|
|
|||
| fun main(args: Array<String>) { | |||
There was a problem hiding this comment.
had to make this one ignored, or it would be detected
There was a problem hiding this comment.
We might want to ignore the main method declaration.
| @@ -103,17 +107,14 @@ class UnusedPrivateMember(config: Config = Config.empty) : Rule(config) { | |||
| } | |||
|
|
|||
| override fun visitProperty(property: KtProperty) { | |||
| if ((property.isPrivate() && property.isNonNestedMember()) | |||
| if ((property.isPrivate() && property.isMemberOrTopLevel()) | |||
There was a problem hiding this comment.
I think @Mauin could help out here.
| @@ -80,6 +80,6 @@ val stuff = object : Iterator<String?> { | |||
| override fun hasNext(): Boolean = true | |||
| } | |||
|
|
|||
| fun main(args: Array<String>) { | |||
There was a problem hiding this comment.
We might want to ignore the main method declaration.
| @@ -95,25 +96,33 @@ class UnusedPrivateMember(config: Config = Config.empty) : Rule(config) { | |||
| } | |||
| } | |||
|
|
|||
| override fun visitPrimaryConstructor(constructor: KtPrimaryConstructor) { | |||
There was a problem hiding this comment.
Added this functionality (and think it's one of the most useful ones!.. me and my team usually do a bad job at removing no-longer used parameters from constructors)
There was a problem hiding this comment.
Definitely! I like this feature.
|
|
||
| private val properties = mutableMapOf<String, KtElement>() | ||
| private val properties = mutableSetOf<KtNamedDeclaration>() |
There was a problem hiding this comment.
Using a map here no longer works, as the visitor could be looking at clashing names in different scopes. We can just use a set of the declarations, filter it before returning it, and just report from there
|
Is this still work in progress? If not, I’ll approve this PR then. |
|
@schalkms sorry.. no, I think this PR is good to go. I'll work on adding unused class detection in a separate one |
This is one approach to allow
UnusedPrivateMemberto look at top level declarations as well.Initial discussion in #948