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 NullableBooleanCheck rule #4872
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4872 +/- ##
=========================================
Coverage 84.79% 84.80%
- Complexity 3447 3463 +16
=========================================
Files 492 493 +1
Lines 11315 11353 +38
Branches 2083 2089 +6
=========================================
+ Hits 9595 9628 +33
- Misses 673 674 +1
- Partials 1047 1051 +4
Continue to review full report at Codecov.
|
The code snippets used in tests are compiled with Jsr223 in this repo to ensure that those code snippets are valid Kotlin code. Some of the code snippets in NullableBooleanCheckSpec seem to be invalid according to the CI run for this PR. Please make sure to correct the code. Thank you for this nice addition resulting from an official Kotlin convention! |
Should be all set now, thanks! |
That’s because of the increased time for test runs. If you run all the tests locally with the compile-snippet-tests setting enabled, it takes quite some time. Hence, we disabled it by default.
|
I think that keep this disabled by default on local is good. But it's true that a lot of new contributors are "bitten" byt this issue. I'm going to open a new issue to look for a way to fix it. |
We can continue this discussion here: #4874. Any feedback is more than welcome. |
...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheck.kt
Outdated
Show resolved
Hide resolved
...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheck.kt
Outdated
Show resolved
Hide resolved
...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/NullableBooleanCheck.kt
Outdated
Show resolved
Hide resolved
Add a new rule which enforces the recommendation of the Kotlin coding conventions to prefer `==` over `?:` when converting a `Boolean?` into a `Boolean`: https://kotlinlang.org/docs/coding-conventions.html#nullable-boolean-values-in-conditions. This is simple to implement and enforces consistency for the two essentially equivalent methods of coalescing nullable boolean values. The coding conventions only specify this preference for usages "in a conditional statement" but the same rationale applies to any statement which converts a Boolean? to a Boolean, so I have implemented it for any situation. An alternative is to add a configuration setting to toggle application only in if statements. Unfortunately, this rule requires type resolution since there are technically valid usages of e.g. `value ?: true` for cases where `value` is not a `Boolean?` but an `Any?`. Running without type resolution will not be able to distinguish between these valid usages and ones which could be converted to `== false`.
Thank you for this rule. It's really nice :) |
I'm not sure if this is the best place to ask, since the change is merged, but I'm wondering if this rule oversteps the intention of the kotlin style guide. The kotlin style guideline linked in the change says
(Emphasis mine.) To me this is fairly clearly talking only about conditional statements such as fun nonNullBoolean() = nullableBoolean() ?: false vs fun nonNullBoolean() = nullableBoolean() == true In the former I can immediately see what the returned value is in the case that |
These are very good points; the reason why we adopted this style on my team is purely as an anti-bikeshedding measure. I would actually also prefer |
I agree with both. It's true that the Kotlin style guideline only talks about conditional statments. But I agree with @dzirbel here that use it everywhere makes the code more consistent. Anyway, if someone is willing to add a flag that, when enabled, only checks inside conditional statments I would be happy to merge it. |
Nice check, found few violations in our project and Thanks for your work @dzirbel! |
Add a new rule which enforces the recommendation of the Kotlin coding conventions to prefer
==
over?:
when converting aBoolean?
into aBoolean
: https://kotlinlang.org/docs/coding-conventions.html#nullable-boolean-values-in-conditions. This is simple to implement and enforces consistency for the two essentially equivalent methods of coalescing nullable boolean values.The coding conventions only specify this preference for usages "in a conditional statement" but the same rationale applies to any statement which converts a Boolean? to a Boolean, so I have implemented it for any situation. An alternative is to add a configuration setting to toggle application only in if statements.
Unfortunately, this rule requires type resolution since there are technically valid usages of e.g.
value ?: true
for cases wherevalue
is not aBoolean?
but anAny?
. Running without type resolution will not be able to distinguish between these valid usages and ones which could be converted to== false
.