Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Update Detekt Configuration] Fine-Tune Default Rules #16984

Merged
merged 12 commits into from
Aug 4, 2022
Merged

[Update Detekt Configuration] Fine-Tune Default Rules #16984

merged 12 commits into from
Aug 4, 2022

Conversation

ParaskP7
Copy link
Contributor

@ParaskP7 ParaskP7 commented Aug 2, 2022

Parent: #16892
Closes: #14011

This long overdue PR, and following @malinajirka's suggestions here, includes the following:

  • Enables the GlobalCoroutineUsage rule (a70d9f7).
  • Enables the DataClassShouldBeImmutable rule (f555742).
  • Enables the SpacingBetweenPackageAndImports rule (edcaa16).
  • Enables the UnusedImports rule (c11bb66).
  • Disables the allowElseExpression configuration for the MissingWhenCase rule (91a5dd8).
  • Adds back the TODO value to the ForbiddenComment rule, by reverting to using the default configuration (8aa70c8).
  • Removes the java.util.* value from the excludeImports configuration for the WildcardImport rule, effectively makes sure that no imports are excluded (6a58246).

Also, this PR aligns some of WPAndroid with the WCAndroid Detekt configuration (see detekt.yml), which includes the following:

  • Enables the ignoreDefaultParameters configuration for the LongParameterList rule (6f3a542). As part of that change all such entries from the baseline got either removed or moved inline and close to source.
  • Disables the TooManyFunctions rule (57cfec8). As part of that change all such entries from the baseline got either removed or moved inline and close to source.
  • Update baseline (57ab46a + 7289664)

PS.1: This PR removes 172 entries from the baseline, which effectively leaves the baseline.xml file with less than 300 entries long.
PS.2: I recommend reviewing this PR commit-by-commit.

To test:

  • There is nothing much to test here.
  • Verifying that all the CI checks are successful should be enough (especially the detekt check).

Regression Notes

  1. Potential unintended areas of impact

N/A

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

See To test section above.

  1. What automated tests I added (or what prevented me from doing so)

N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

ParaskP7 added 11 commits August 2, 2022 11:02
@ParaskP7
Analysis: Enable global coroutine usage detekt rule
a70d9f7 
Enabling this rule was discussed in the link shared below and thus this
is a good opportunity to fine-tune Detekt to include it:

Link: #14011
#issuecomment-775885924

Also, as part of enabling this rule, the only warning based on that was
suppressed, that is, instead of being resolved since a resolution would
require a proper solution (and testing).
@ParaskP7
Analysis: Enable data class should be immutable detekt rule
f555742 
Enabling this rule was discussed in the link shared below and thus this
is a good opportunity to fine-tune Detekt to include it:

Link: #14011
#issuecomment-775885924

Also, as part of enabling this rule, some warnings based on that were
suppressed, that is, instead of being resolved since a resolution would
require a proper solution (and testing), while some other warnings were
properly resolved.
@ParaskP7
Analysis: Enable spacing between package and imports detekt rule
edcaa16 
Enabling this rule was discussed in the link shared below and thus this
is a good opportunity to fine-tune Detekt to include it:

Link: #14011
#issuecomment-775885924
@ParaskP7
Analysis: Enable unused imports detekt rule
c11bb66 
Enabling this rule was discussed in the link shared below and thus this
is a good opportunity to fine-tune Detekt to include it:

Link: #14011
#issuecomment-775885924

Also, as part of enabling this rule, the only warning based on that was
resolved.
@ParaskP7
Analysis: Disable allow else expression for the missing when case rule
91a5dd8 
Disabling this configuration was discussed in the link shared below and
thus this is a good opportunity to fine-tune Detekt to update it:

Link: #14011
#issuecomment-775885924
@ParaskP7
Analysis: Add back the default todo value for the forbidden comment rule
8aa70c8 
Updating this rule was discussed in the link shared below and thus this
is a good opportunity to fine-tune Detekt to update it:

Link: #14011
#issuecomment-775885924

Also, as part of enabling this rule, some warnings based on that was
suppressed (them being TODOs), that is, instead of being resolved since
a resolution would require a proper solution (and testing).

PS: This commit align WPAndroid with WCAndroid in terms of Detekt rules.
@ParaskP7
Analysis: Remove exclude imports config from the wildcard import rule
6a58246 
Updating this rule was discussed in the link shared below and thus this
is a good opportunity to fine-tune Detekt to update it:

Link: #14011
#issuecomment-775885924
@ParaskP7
Analysis: Enable ignore default parameters for the long param list rule
6f3a542 
Updating this rule to align WPAndroid with WCAndroid in terms of Detekt
rules.

As part of updating this rule you will notice that all such baseline
rules got removed and either added explicitly, inline/close to source or
just deleted since they are no longer necessary. Thus, you will notice
some new suppress annotations that got added as part of this commit.
But, all those warnings are being added because they got moved from the
baseline and not because they are new.
@ParaskP7
Analysis: Disable too many functions detekt rule
57cfec8 
Disabling this rule to align WPAndroid with WCAndroid in terms of Detekt
rules.

As part of disabling this rule you will notice that all such baseline
rules got removed as well.
@ParaskP7
Analysis: Move long method warnings from baseline and close to source
57ab46a 
This was done because those warnings are line based and since these
files got updated from previous commits they are now complaining again.
Thus, removing them from the baseline and moving them explicitly,
inline/close to source makes it so that this problem will never happen
again for those specific cases.
@ParaskP7
Analysis: Move complex method warnings from baseline and close to source
7289664 
This was done because those warnings are line based and since these
files got updated from previous commits they are now complaining again.
Thus, removing them from the baseline and moving them explicitly,
inline/close to source makes it so that this problem will never happen
again for those specific cases.
@ParaskP7 ParaskP7 added this to the 20.5 milestone Aug 2, 2022
@ParaskP7 ParaskP7 requested review from zwarm and a team August 2, 2022 08:41
@ParaskP7 ParaskP7 self-assigned this Aug 2, 2022
@ParaskP7 ParaskP7 mentioned this pull request Aug 2, 2022
Closed
11 tasks
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 2, 2022 •
edited
Loading

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr16984-2fa2522.apk), or scanning this QR code:

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Aug 2, 2022 •
edited
Loading

You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr16984-2fa2522.apk), or scanning this QR code:

zwarm
zwarm approved these changes Aug 4, 2022
View reviewed changes
Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ParaskP7 🙏 🙇 Thanks for wrangling this. I look forward to seeing this in action once it is merged to trunk. Lots of good stuff in here!!

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Aug 4, 2022

Great stuff, thank you so much for the review @zwarm ! 🙇

I am going to merge the latest trunk to this PR once more, just to make sure that any new code since isn't violating the new configuration and then I am going to merge it, I am so looking forward to that!

@ParaskP7
Merge branch 'trunk' of github.com:wordpress-mobile/WordPress-Android…
2fa2522 
… into analysis/detekt-fine-tune-default-rules
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 4, 2022 •
edited
Loading

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@ParaskP7 ParaskP7 added Tooling and removed Tooling labels Aug 4, 2022
@ParaskP7 ParaskP7 merged commit f53eea9 into trunk Aug 4, 2022
@ParaskP7 ParaskP7 deleted the analysis/detekt-fine-tune-default-rules branch August 4, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zwarm zwarm zwarm approved these changes

Assignees

@ParaskP7 ParaskP7

Labels
Tooling
Projects
None yet
Milestone
20.5
Development

Successfully merging this pull request may close these issues.

Detekt Integration - Phase.2
3 participants
@ParaskP7 @wpmobilebot @zwarm