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.

Sign up for GitHub

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

Jump to bottom

Reduce configuration of UnusedPrivateMember's split rules #5800

Merged
merged 4 commits into from
Feb 24, 2023
Merged

Reduce configuration of UnusedPrivateMember's split rules #5800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a8f2c88
_ is not a valid name for fuction or property
BraisGabin Feb 21, 2023
28b383c
serialVersionUID is not a reserved name for parameters or functions
BraisGabin Feb 21, 2023
f603cd7
Remove redundant parenthesis
BraisGabin Feb 21, 2023
562d9c7
There is no function name that should be ignored by default
BraisGabin Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions detekt-core/src/main/resources/default-detekt-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,15 +705,15 @@ style:
active: false
UnusedParameter:
active: true
allowedNames: '(_|ignored|expected|serialVersionUID)'
allowedNames: 'ignored|expected'
UnusedPrivateClass:
active: true
UnusedPrivateMember:
active: true
allowedNames: '(_|ignored|expected|serialVersionUID)'
allowedNames: ''
UnusedPrivateProperty:
active: true
allowedNames: '(_|ignored|expected|serialVersionUID)'
allowedNames: '_|ignored|expected|serialVersionUID'
Comment on lines 706 to +716
Copy link
Member

Choose a reason for hiding this comment

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

feels like _ should be a parameter name, i.e.

foo.bar { _, x -> ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

It does... we could open an issue and move that to the other rule.

also I created this test case and we have a lot of false negatives regarding lambdas:

Pair(1, 2).let { (x, y) -> println(y) }
listOf<Int>().map { x -> 1 }.fold(a) { x, y -> y }
val foo: (String, String) -> Unit = { x, y -> println(y) }

And I think that we don't even should configure _ as an unused parameter. That's exactly the reason _ exists.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but otherwise it would be a false positive, isn't it?

I didn't know there were so many gaps, my point was that you added _ as private property, but not for parameter names, now I tried and it's not allowed:
image
image
image

so it's pretty much only allowed as a lambda parameter.

Copy link
Member

Choose a reason for hiding this comment

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

@BraisGabin based on ^, it looks like we should just remove the _ exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't remove it from the property rule because, otherwise, we have false positives right now. We should probably take a look at it. I don't think the lambda check should be in the property rule and I think that we should omit those by default. But that's an improvement out of scope for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see, the lambda param is a "private property", sorry, I missed that.

UseAnyOrNoneInsteadOfFind:
active: true
UseArrayLiteralsInAnnotations:
Expand Down
2 changes: 1 addition & 1 deletion ...kt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedParameter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class UnusedParameter(config: Config = Config.empty) : Rule(config) {
)

@Configuration("unused parameter 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", String::toRegex)

override fun visit(root: KtFile) {
super.visit(root)
Expand Down
3 changes: 2 additions & 1 deletion ...ules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateMember.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ private const val ARRAY_GET_METHOD_NAME = "get"

/**
* Reports unused private functions.
*
* If these private functions are unused they should be removed. Otherwise, this dead code
* can lead to confusion and potential bugs.
*/
Expand All @@ -55,7 +56,7 @@ class UnusedPrivateMember(config: Config = Config.empty) : Rule(config) {
)

@Configuration("unused private function names matching this regex are ignored")
private val allowedNames: Regex by config("(_|ignored|expected|serialVersionUID)", String::toRegex)
private val allowedNames: Regex by config("", String::toRegex)

override fun visit(root: KtFile) {
super.visit(root)
Expand Down
2 changes: 1 addition & 1 deletion ...es-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateProperty.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class UnusedPrivateProperty(config: Config = Config.empty) : Rule(config) {
)

@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", String::toRegex)

override fun visit(root: KtFile) {
super.visit(root)
Expand Down
14 changes: 0 additions & 14 deletions ...-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UnusedPrivateMemberSpec.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,20 +278,6 @@ class UnusedPrivateMemberSpec(val env: KotlinCoreEnvironment) {
}
}

@Nested
inner class `unused class declarations which are allowed` {

@Test
fun `does not report the unused private function and parameter`() {
val code = """
class Test {
private fun ignored(ignored: Int) {}
}
""".trimIndent()
assertThat(subject.lint(code)).isEmpty()
}
}

@Nested
inner class `error messages` {

Expand Down