[Update Detekt Version] Update Detekt to 1.21.0 #16961
Conversation
With this update, the configuration for the ruleset below got updated. Also, a couple of rules got fixed: Ruleset that got updated: - formatting Rules that got fixed: - TopLevelPropertyNaming (naming) - UnnecessaryAbstractClass (style) As such, for the above listed ruleset and all the above listed rules, Detekt has started to warn about current violations based on these updates. As such those need to be updated accordingly. This is going to be done in subsequent commits.
With the '1.20.0' Detekt update, the Detekt 'formatting' section got moved out of the default config and into its own 'formatting' related configuration. This is done because the 'formatting' ruleset provided by Detekt is updated to use a new mechanism that allow authors to define the default configuration for their custom rules, which will then be merged together with the user configuration and can be overridden by the user if they wish. Formatting Config: https://github.com/detekt/detekt/blob/main/ detekt-formatting/src/main/resources/config/config.yml Actually, if this is not done, then, when running the './gradlew detekt' task it fails with the below message: "* What went wrong: Execution failed for task ':WordPress:detekt'. > Run failed with 1 invalid config property. - Property 'formatting' is misspelled or does not exist."
Warning Message: "An abstract class without a concrete member can be refactored to an interface." For more info see: https://detekt.dev/docs/rules/style/ #unnecessaryabstractclass
Warning Message: "Top level constant names should match the pattern: [A-Z][_A-Z0-9]*" For more info see: https://detekt.dev/docs/rules/naming/ #toplevelpropertynaming
With this update, some rules, like the below, which were disabled by default, are now enabled by default instead: - UseCheckOrError (style) - ExplicitItLambdaParameter (style) - UseRequire (style) - DestructuringDeclarationWithTooManyEntries (style) Also, the below rules got fixed: - ReturnCount (style) As such, for all the above listed rules, Detekt has started to warn about current violations based on these rules. As such those need to be resolved/suppressed accordingly. This is going to be done in subsequent commits.
Warning Message: "Use check() or error() instead of throwing an IllegalStateException." For more info see: https://detekt.dev/docs/rules/style/#usecheckorerror
Warning Message: "This explicit usage of `it` as the lambda parameter name can be omitted." For more info see: https://detekt.dev/docs/rules/style/ #explicititlambdaparameter
Warning Message: "Use require() instead of throwing an IllegalArgumentException." For more info see: https://detekt.dev/docs/rules/style/#userequire
Warning Message: "The destructuring declaration contains 4 but only 3 are allowed." For more info see: https://detekt.dev/docs/rules/style/ #destructuringdeclarationwithtoomanyentries
Warning Message: "Function shouldOpenExternal has 3 return statements which exceeds the limit of 2." For more info see: https://detekt.dev/docs/rules/style/#returncount
Warning Message: "Function names should match the pattern: [a-z][a-zA-Z0-9]*" For more info see: https://detekt.dev/docs/rules/naming/#functionnaming
Warning Message: "The function trackEvent appears to be too complex (20). Defined complexity threshold for methods is set to '15'" For more info see: https://detekt.dev/docs/rules/complexity/ #complexmethod
Suggestion: "Customise the reports on the Detekt task(s) instead." For more info see: https://detekt.dev/docs/gettingstarted/gradle#reports However, since the default for all of them (html, xml and txt) is true anyway, removing this deprecated Detekt reports configuration might be the best alternative in order not to pullute the 'build.gradle' with an extra task configuration. This change also makes the overall Detekt configuration less complex.
You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr16961-0b82259.apk), or scanning this QR code:
|
You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr16961-0b82259.apk), or scanning this QR code:
|
|
@ParaskP7 FYI: I won't be able to get to this until Monday because I need a couple days of focused project work. If it's still up for review, I should be able to review it then 🤞 |
There was a problem hiding this comment.
@ParaskP7 I have reviewed the changes in the PR commit-by-commit. Everything looks good to me. I have done smoke testing of the app as well. Works fine 👍🏼
👏🏼 Praise: Thanks for adding the detailed messages in each commit and also for the links to each rule set in the corresponding commit. You 🪨
I am approving the changes but leaving this unmerged in case you're waiting for more feedback from the https://github.com/orgs/wordpress-mobile/teams/owl-team.
Feel free to merge this at your convenience.
|
👋 @oguzkocer !
Hey, thanks for the heads-up and for being so considerate! 🙇 In this case, please don't worry about this PR, is nothing too urgent. That's one of the reasons I am always choosing to also add one member of the client engineering team (as the main reviewer) and us, the Owl team in general, as an additional one. thank YOU Oguz! 🥇 And... I am just seeing, that Mr. @AjeshRPai the great, already did the review, thank YOU Ajesh! 🥇 |
|
👋 @AjeshRPai !
Great, thank you so much for the review and smoke testing Ajesh, you rock! 🙇 🌟 🪨
Back to you! ❤️
Coolio, your approval is all I need at this point, just one approval, thank you so so so much! 🙏
👍 |
Parent: #16892
Closes: #16898
Closes: #16899
This PR:
1.21.0(from1.19.0via1.20.0) (f9803cb + bb316e9).UnnecessaryAbstractClasswarnings (d702ca9)TopLevelPropertyNamingwarnings (4ef2603)UseCheckOrErrorwarnings (bd4df02)ExplicitItLambdaParameterwarnings (3046bb7)UseRequirewarnings (0983a08)DestructuringDeclarationWithTooManyEntrieswarnings (e634652)ReturnCountwarnings (74baf6e)FunctionNamingwarnings (8ba62d7)ComplexMethodwarning (bd067b6)PS: I recommend reviewing this PR commit-by-commit.
To test:
Regression Notes
N/A
See
To testsection above.N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.