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

Detekt Integration - Phase.2 #14011

Closed
ParaskP7 opened this issue Feb 9, 2021 · 3 comments · Fixed by #16984
Closed

Detekt Integration - Phase.2 #14011

ParaskP7 opened this issue Feb 9, 2021 · 3 comments · Fixed by #16984
Assignees
@malinajirka
Labels
Tooling [Type] Task

Comments

@ParaskP7
Copy link
Contributor

ParaskP7 commented Feb 9, 2021

Relates #13996

This issue is about integrating Detekt, phase by phase, into the WordPress-Android codebase.

Phase.1 was actually adding Detekt alongside its default behavior into the WordPress-Android codebase. This relates #13996 PR is responsible for that.

This Phase.2 will be all about fine-tuning the default rules to utilise Detekt as much as possible. In this issue every developer is encourage to suggest the rules they would like to see enabled (or disabled). Please comment on this issue with your suggestion(s).
@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Feb 9, 2021

Suggestions from @malinajirka based on this #13996 PR's code review:

  1. Activate the GlobalCoroutineUsage rule.
  2. Deactivate the allowElseExpression sub-rule.
  3. Activate the DataClassShouldBeImmutable rule.
  4. Add back the TODO value into the ForbiddenComment rule and instead utilise the allowedPatterns to only allow TODOs with a deadline and an assigned developer.
  5. Activate the ForbiddenImport rule with .* as the forbidden pattern. PS: KtLint already does that.
  6. Activate the SpacingBetweenPackageAndImports rule. PS: KtLint already does that.
  7. Activate the UnusedImports rule. PS: KtLint already does that.

This was referenced Feb 9, 2021
Merged
Closed
@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Feb 22, 2021 •
edited
Loading

Here is another suggestion from @planarvoid on the LongParameterList, see internal link: paqN3M-hh-p2#comment-481

@ParaskP7
Copy link
Contributor Author

ParaskP7 commented Feb 25, 2021 •
edited
Loading

  • Here is another suggestion from @khaykov on the TooManyFunctions, see internal link: paqN3M-hh-p2#comment-489
  • Here is another suggestion from @JorgeMucientes on the TooManyFunctions, see internal link: p91TBi-6jX-p2

ovitrif added a commit that referenced this issue Jun 24, 2022
@ovitrif
Apply suggestions 2, 6 & 7 from the list of 7 in comment of #14011
e3a8a46 
See #14011 (comment)
@ovitrif ovitrif mentioned this issue Jun 24, 2022
Closed
3 tasks
@ParaskP7 ParaskP7 mentioned this issue Jul 12, 2022
Closed
11 tasks
ParaskP7 added a commit that referenced this issue Aug 2, 2022
@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 added a commit that referenced this issue Aug 2, 2022
@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 added a commit that referenced this issue Aug 2, 2022
@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 added a commit that referenced this issue Aug 2, 2022
@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 added a commit that referenced this issue Aug 2, 2022
@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 added a commit that referenced this issue Aug 2, 2022
@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 added a commit that referenced this issue Aug 2, 2022
@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 ParaskP7 mentioned this issue Aug 2, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@malinajirka malinajirka

Labels
Tooling [Type] Task
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Update Detekt Configuration] Fine-Tune Default Rules wordpress-mobile/WordPress-Android
3 participants
@designsimply @malinajirka @ParaskP7