MultilineLambdaItParameter: fix false positive for one-line statements with a lambda argument #5505
Conversation
…s with a lambda argument
| @@ -106,4 +110,9 @@ class MultilineLambdaItParameter(val config: Config) : Rule(config) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| private fun KtExpression.hasNoLineBreak() = !textContains('\n') | |||
There was a problem hiding this comment.
You're assuming file separators, which will work for most, but some might use \r only.
Is it possible to get the beginning/ending line number of the PsiElement and calculate how many lines it is?
There was a problem hiding this comment.
Line separators are converted to \n.
There was a problem hiding this comment.
Hmm, a related thought: you're assuming here that Detekt compiles the PSI, but what if it's run in a different environment? #5492 at that point all assumptions about line breaks will fail in Detekt rules. cc @3flex
I think checking the line numbers to detect single line is still the most versatile solution that can be also adopted by others when they find this code and copy it.
...le/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineLambdaItParameterSpec.kt
Outdated
Show resolved
Hide resolved
...le/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/MultilineLambdaItParameterSpec.kt
Show resolved
Hide resolved
| val single = statements.singleOrNull() | ||
| if (single != null && (single.hasNoLineBreak() || single.hasNoStatements())) return |
There was a problem hiding this comment.
It feels like semantically this might be cleaner (return early + renames), but up to you:
| val single = statements.singleOrNull() | |
| if (single != null && (single.hasNoLineBreak() || single.hasNoStatements())) return | |
| val statement = statements.singleOrNull() ?: return | |
| if (statement.isSingleLine() || statement.hasNoSubStatements()) return |
There was a problem hiding this comment.
val statement = statements.singleOrNull() ?: return
We can't return here because we need to flag the case of multiple statements.
There was a problem hiding this comment.
Oh, have to remember to expand the code below the changed lines. Agreed. Please consider renames anyway.
|
Amazingly fast response @t-kameyama 👏🏻! |
|
cc @eygraber |
| fun f(i: Int?) { | ||
| val foo = i?.let { | ||
| Foo( | ||
| x = it, |
There was a problem hiding this comment.
I'm a little less sure about this case, since it's multiline and the it isn't the first line.
Not sure if the example is too generic, but I'm trying to look at this test through the lens of a reader not familiar with the code, and it's not immediately clear to me what the it represents here. Although I could think of examples where it is clear.
There was a problem hiding this comment.
Agree with @eygraber . The following block is non-compliant according to the kDoc.
val digits = 1234.let { it ->
println(it)
listOf(it)
}
Let's keep the behavior to be consistent with the spec name MultilineLambdaItParameter
Fixes #5504