-
-
Notifications
You must be signed in to change notification settings - Fork 794
enable more potential-bugs rules for detekt code base #3997
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
enable more potential-bugs rules for detekt code base #3997
Conversation
override fun shutdown() = Unit | ||
override fun shutdown(): Unit = Unit |
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.
Fun fact:
override fun shutdown() = Unit // -> violates ImplicitUnitReturnType
override fun shutdown(): Unit = Unit // -> violates OptionalUnit
override fun shutdown() {} // -> violates EmptyFunctionBlock
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.
override fun shutdown() = Unit // -> violates ImplicitUnitReturnType
This feels like a false positive, right?
It's not "Implicit".
Anyway I like this way to implement it:
override fun shutdown() {
// no-op
}
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.
I'd vote to revert this change (and the one in LicenceHeaderLoaderExtensionSpec.kt). Add them to our baseline and open an issue to fix the false-positive. Those changes doesn't improve the code so they should not be made just to make detekt happy.
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.
Those changes doesn't improve the code so they should not be made just to make detekt happy.
Agree +1. I also prefer the //no-op
approach but the status quo ( = Unit
) should also be accepted.
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.
I added @Suppress
to those 2 occurrences and raised #4004
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.
Is it good to add the suppress? For false postives the suppress have the problem that generates debt because we will need to remove it. If we use the baseline instead we can just regenerate it in the next version.
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.
Agreed and changed. Unfortunately gradlew detektBaselineTest
will be run for each module and output the baseline to config/detekt/baseline-test.xml
. So each module overwrites the baseline from the previous module making it a rather manual process to create a combined version.
Codecov Report
@@ Coverage Diff @@
## main #3997 +/- ##
============================================
- Coverage 83.55% 83.53% -0.02%
Complexity 3179 3179
============================================
Files 459 459
Lines 9085 9082 -3
Branches 1765 1772 +7
============================================
- Hits 7591 7587 -4
+ Misses 564 561 -3
- Partials 930 934 +4
Continue to review full report at Codecov.
|
This relates to #3993 and enables checks from the potential-bugs rule set