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
Add UseLet style rule #6091
Add UseLet style rule #6091
Conversation
detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseLetSpec.kt
Fixed
Show fixed
Hide fixed
detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseLetSpec.kt
Fixed
Show fixed
Hide fixed
detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseLetSpec.kt
Fixed
Show fixed
Hide fixed
detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/UseLetSpec.kt
Fixed
Show fixed
Hide fixed
Codecov Report
@@ Coverage Diff @@
## main #6091 +/- ##
============================================
+ Coverage 84.83% 84.86% +0.02%
- Complexity 3997 4011 +14
============================================
Files 567 568 +1
Lines 13400 13422 +22
Branches 2362 2368 +6
============================================
+ Hits 11368 11390 +22
Misses 871 871
Partials 1161 1161
|
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.
LGTM. Just remove the documentation update so we can talk about it in other pr because I'm not 100% sure about it. But that shouldn't block the rest of the pr.
...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UselessCallOnNotNull.kt
Outdated
Show resolved
Hide resolved
Thank you for the rule. It looks really good. I know this style of tests is really popular but to me it feels harder to read than necessary and hard to maintain. But I may be the only one to see it that way. I agree with @BraisGabin that the documentation change should be its own pr. |
I agree they are hard to read. But test all the cases possible cases is really nice. All those tests would be a nightmare to maintain too if they are duplicated time over time... It's a difficult compromise. |
I think I might have found a false positive. fun testCallToCreateTempFile(s: String?) {
val x = if (s == null) {
println("foo")
null
} else {
"x"
}
} In my opinion this cannot be easily rewritten using |
@marschwar I agree that that is the exception case. We are using that as the times to use It was pointed out that you could rewrite that as |
Hi, @tresni thanks for contributing towards the |
Yes. And I think that with that, and a test to demonstrate it, this rule is ready to merge. |
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.
Great work.
@detekt/maintainers We could enable it for the detekt codebase right away. There are no findings.
@tresni Could you please activate your rule in the |
I asked a question about my rule in the #detekt channel, and there seemed to be some appetite for the rule. This is what my rule currently looks like.
I found that in a code base I was working with, there were a lot of instances of
if (x != null) { do_something(x) } else null
and wanted to encourage the use of?.let
in this specific case. The plugin also looks forif (x == null) { null } else do_something(x)
as we had some of that too.