Enable more rules by default #3229
Conversation
| @@ -468,11 +468,11 @@ potential-bugs: | |||
| HasPlatformType: | |||
| active: false | |||
| IgnoredReturnValue: | |||
| active: false | |||
| active: true | |||
There was a problem hiding this comment.
I'm unsure about this. We had several false positives for this rules, specifically from RxJava users.
|
The added rules look good to me 👍 We have also some more rules enabled in our analysis which are not active by default: |
That's okay for me. I'll also check the ones mentioned in your last comment. |
Codecov Report
@@ Coverage Diff @@
## master #3229 +/- ##
============================================
+ Coverage 80.43% 80.48% +0.04%
+ Complexity 2740 2739 -1
============================================
Files 448 448
Lines 8281 8275 -6
Branches 1573 1573
============================================
- Hits 6661 6660 -1
+ Misses 772 767 -5
Partials 848 848
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
As @cortinico said we need to add the @active. Then we could just regenerate the file and the values should be changed automatically.
Yep, this is the 2nd step. The 1st step should be the draft of the activated rules.
Regarding the typos, it’s probably not a good idea to edit the config on the go with a smartphone web browser. Well, we luckily have a good CI system and carefully observing reviewers for those reasons. Cheers and have a nice weekend, |
|
What do you think about enable the coroutine ones? I think that all are quite safe. The more controversial is So my list to be enabled is:
Not so sure:
|
This way users can benefit from a good default config. Closes #2838
|
Thanks for your work, @BraisGabin.
|
schalkms
force-pushed
the
schalkms-enable_rules
branch
from
7a468fd
to
b412321
Compare
I agree with you. We should not enable the rules you list. |
Agree on this
I think this should not be enabled as it's a rule users have to configure, with the list of Methods they want to restrict. |
We have two default values there for println. And I think that we should forbid that for default. |
| * | ||
| * @active since v1.16.0 |
There was a problem hiding this comment.
I was wondering if we can add some documentation or update the existing documentation for why we enable certain rules. Below is a copy of the comment I had:
Printing a stacktrace may be useful for local development, but not for production code.
There was a problem hiding this comment.
Every rule should have a descripction so any user can know why he/she should enable/disable it. If you think that in this case (or any other) the description is not good enough you can create an issue. Or, even better, if you know how to improve it, create a PR.
But I don't think that we should add any documentation about why a rule is or is not active by default.
| @@ -37,6 +37,8 @@ import org.jetbrains.kotlin.psi.KtProperty | |||
| * } | |||
| * } | |||
| * </compliant> | |||
| * | |||
| * @active since v1.16.0 | |||
There was a problem hiding this comment.
Note that Android context, Parcelable is preferred over Serializable. This rule is almost a no-op for Android code.
There was a problem hiding this comment.
I agree that in Android we should not use Serializable, but, if we have, we should use the SerialVersionUID. So I think that it's safe to have this enabled.
We talked some times about an Android rule set. A good rule for there would be to flag any use of Serializable.
There was a problem hiding this comment.
I just create a discussion to talk about it: #3303
|
@chao2zhang thanks for pushing this over the finish line. |
… it's available at maven central 1.17.0 (and its dependencies) is also available at maven central (see https://detekt.github.io/detekt/changelog.html#notable-changes-2), but may as well pick up the latest 1.17.x. See https://detekt.github.io/detekt/changelog.html#changelog-3 / detekt/detekt#3229. SwallowedException and ImplicitDefaultLocale rules are now enabled by default, so fix the warnings they generate. Use Locale.US because other code in kork and orca do.
… it's available at maven central 1.17.0 (and its dependencies) is also available at maven central (see https://detekt.github.io/detekt/changelog.html#notable-changes-2), but may as well pick up the latest 1.17.x. See https://detekt.github.io/detekt/changelog.html#changelog-3 / detekt/detekt#3229. SwallowedException and ImplicitDefaultLocale rules are enabled by default as of version 1.16.0, so fix the warnings they generate. Use Locale.US because other code in kork and orca do.
* chore(build): use dependencies that exist in maven central some dependencies (or transitive dependencies of those) only existed in bintray, not maven central, and now that bintray is gone, we need to adjust. * chore(build): use version 1.17.1 of io.gitlab.arturbosch.detekt since it's available at maven central 1.17.0 (and its dependencies) is also available at maven central (see https://detekt.github.io/detekt/changelog.html#notable-changes-2), but may as well pick up the latest 1.17.x. See https://detekt.github.io/detekt/changelog.html#changelog-3 / detekt/detekt#3229. SwallowedException and ImplicitDefaultLocale rules are enabled by default as of version 1.16.0, so fix the warnings they generate. Use Locale.US because other code in kork and orca do.
…#873) * chore(build): use dependencies that exist in maven central some dependencies (or transitive dependencies of those) only existed in bintray, not maven central, and now that bintray is gone, we need to adjust. * chore(build): use version 1.17.1 of io.gitlab.arturbosch.detekt since it's available at maven central 1.17.0 (and its dependencies) is also available at maven central (see https://detekt.github.io/detekt/changelog.html#notable-changes-2), but may as well pick up the latest 1.17.x. See https://detekt.github.io/detekt/changelog.html#changelog-3 / detekt/detekt#3229. SwallowedException and ImplicitDefaultLocale rules are enabled by default as of version 1.16.0, so fix the warnings they generate. Use Locale.US because other code in kork and orca do.
This is based on spinnaker#873 and spinnaker#846 with some tweaks since kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/v2/SpringEventListenerAdapter.kt doesn't exist in our version of kork. * chore(build): use dependencies that exist in maven central some dependencies (or transitive dependencies of those) only existed in bintray, not maven central, and now that bintray is gone, we need to adjust. * chore(build): use version 1.17.1 of io.gitlab.arturbosch.detekt since it's available at maven central 1.17.0 (and its dependencies) is also available at maven central (see https://detekt.github.io/detekt/changelog.html#notable-changes-2), but may as well pick up the latest 1.17.x. See https://detekt.github.io/detekt/changelog.html#changelog-3 / detekt/detekt#3229. The ImplicitDefaultLocale rule is enabled by default as of version 1.16.0, so fix the warnings it generates. Use Locale.US because other code in kork and orca do. @W-9762454
This is a first draft for enabling more detekt rules by default.
This way users can benefit from a good default config.
Closes #2838