Detekt Integration - Phase.1 #13996

ParaskP7 · 2021-02-08T15:16:35Z

External Links: Detekt - Detekt Documentation
Internal Link: paqN3M-eg-p2 (Detekt Integration)
Related PR: Showcase
Next Phases: Phase.2 - Phase.3

This PR adds Detekt to the WordPress repo. Detekt is a static code analysis tool for the Kotlin programming language. Detekt does for Kotlin what CheckStyle does for Java, only so much more.

To test (🧪 ):

Make sure to read the external and internal links mention above to get an overall idea on what Detekt has to offer, how it can be configured and utilised.
Pull detekt-integration on a local branch.
Run ./gradlew WordPress:detekt and verify that the build is successful.
Delete the config/detekt/baseline.xml file, comment out the Detekt's baseline configuration within the WordPress/build.gradle file and run ./gradlew WordPress:detekt again. Verify that the build fails, reporting 880 issues.
Install the detekt IDE plugin and configure it following the instructions within the coding-style.md readme file. Verify that everything works and that the IDE reports Detekt violations as well.
Locate the Detekt related Circle CI job and verify correctness.

Questions (❓):

For @loremattei: Detekt was configured on the root build.gradle file, within the allprojects { ... } scope. However, I have noticed that the subprojects { ... } is utilised as well for static analysis tools like KtLint. Do I need to add an additional task for Detekt within the subprojects { ... }?
For @loremattei: The exist a task within the WordPress build.gradle file, called violationCommentsToGitHub. This task has a violations configuration for CheckStyle. Do I need to add something similar for Detekt?

Merge Instructions (⚠️):

Make sure 2 reviewers have approved the PR (@loremattei and @malinajirka).
Remove the Not Ready for Merge label.
Merge this PR.

PR submission checklist:

I have considered adding unit tests where possible.
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.

This commit adds 'Detekt' version '1.15.0', that bundles with Kotlin version '1.4.10', which is the Kotlin version currently utilised within this repo. The next version of 'Detekt' is the '1.16.0-RC1', which updates Kotlin to '1.4.21'. But, this is unnecessary, at the moment, for this repo. Also, it is still a release candidate. In addition to that, this commit configures 'Detekt' using the default configuration, defaulting all options to 'false' for ease of integrating the tool to the repo. The 'detekt.yml' file was automatically generated running the 'detektGenerateConfig' command. This file remained unchanged in order to utilise 'Detekt's' default configuration as a starting point. The only configuration that was updated was the 'warningsAsErrors', which was update to 'true' in order to have all warning being treated as errors by default. Apart from that, the 'detekt-formatting' plugin was also added to this configuration in order to have the 'Formatting' section being reported, alongside all its corresponding formatting violations (currently 45). However, the 'autoCorrect' configuration flag was set to 'false' in order to not have the codebase being automatically reformatted whenever 'Detekt' gets triggered.

This 'Detekt' baseline suppresses the existing issues (880 in total) to have 'Detekt' running as if the codebase has zero issues. This helps to start utilising 'Detekt' immediately without having to worry about the existing issues. Any new issues that will be added to the codebase right after will be reported and the build will fail accordingly.

This 'Circle CI' step differs to the rest since it actually targets a specific module, the 'WordPress' module, instead of running 'Detekt' on all the modules, like the other steps do. This is by design since 'Detekt' is currently configured for the 'WordPress' module only. Locally, every developer will need to also trigger './gradlew WordPress:detekt' instead of './gradlew detekt' as the former will run 'Detekt' on all modules, which is not currently supported.

peril-wordpress-mobile · 2021-02-08T15:16:41Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-02-08T15:28:59Z

You can test the changes on this Pull Request by downloading the APK here.

malinajirka

Thank you for working on this @ParaskP7!

I've tested the configuration and it seems to work as expected. I've left a couple of comments, let me know what you think.

P.S. I installed the IDE plugin and I created a method with 5 returns statements. The cmd task found the error, should the IDE plugin underline the method or somehow indicate an error?

build.gradle

malinajirka · 2021-02-09T08:50:04Z

build.gradle

+        parallel = false
+        debug = false
+        reports {
+            html.enabled = true


💡 Idea: Unless we are planning to use all three formats, I'd personally disabled those we don't need.

👋 @malinajirka !

Yes, I was thinking about that too. I choose to not disable any of the formats for two reasons:

The output files are very small anyway, thus there will not be a build overhead,

I don't know how and which format our CI will going to be using in the future, for reporting purposes, so I enabled all possible formats to make it easier for future integration.

PS: We can definitely have only one of those enabled if you like. Let me know which one you prefer. @loremattei

I think xml is enough for CI as it's the one used by the plugin we use to post comments on PRs. 👍

Many thanks for adding to that @loremattei !

So, since xml is required by CI, and IMHO html is a real nice to have locally, to get a nice representation of the issue at hand and how to fix it, I would have those two enabled. This might leave us with only txt to decide to disable, but even that I suggest to keep it enabled since I find myself sometimes looking at this find to figure what the problem more rapidly. I don't think those files are too much overhead and as such I would have the enabled (at least for now).

@loremattei @malinajirka your thoughts?

PS: I will be fine even if we decide to disable all of them and ending up having xml the only one being enabled.

If it doesn't take any significant amount of time, I agree that keeping all of them is fine.

config/detekt/detekt.yml

ParaskP7 · 2021-02-09T10:32:10Z

👋 @malinajirka !

I've tested the configuration and it seems to work as expected. I've left a couple of comments, let me know what you think.

Thank you so much for this thorough review and all the comments you left Jirka, much much appreciated! 💪

P.S. I installed the IDE plugin and I created a method with 5 returns statements. The cmd task found the error, should the IDE plugin underline the method or somehow indicate an error?

The IDE plugin should highlight the whole line with yellow color and indicate that this is a warning. You want want to restart your IDE or sync it or the plugin might be misbehaving on its current version or due to the new Detekt version, not sure. However, I also noticed that my IDE wasn't highlighting those warnings for me too yesterday, but skipped the debugging for later.

This is done in order to align this configuration with the more global root level 'build.gradle' detekt 'autoCorrect = false' configuration.

This is done in order to make it explicit that whatever additional android formatting rule can be applicable is not being ignore. Note that after enabling this formatting flag and running './gradlew WordPress:detekt' there was no new formatting issue reported.

Since code is being merged behind feature flags into 'develop' TODO comments are unavoidable at the moment. In the future, this rule might get enhanced to only allow TODO comments that contain a deadline and have a developer assigned to them. Also, note that the baseline was update to exclude these TODO related forbidden comments.

Also, note that the baseline was update to exclude these enum related magic numbers.

…id into detekt-integration

Two new issues were added to the baseline.

ParaskP7 · 2021-02-09T13:34:18Z

👋 @malinajirka !

I just re-requested your review on this PR, could you please check out my responses per comment and any new commit that fixes what was described.

Many thanks in advance! 🙏

malinajirka

LGTM, thanks for the changes and for creating the issue for 2nd phase ;).

loremattei

Thank you for this @ParaskP7!

I tested following the steps you provided and it worked as expected, other than what @malinajirka already noted.

Detekt was configured on the root build.gradle file, within the allprojects { ... } scope. However, I have noticed that the subprojects { ... } is utilised as well for static analysis tools like KtLint. Do I need to add an additional task for Detekt within the subprojects { ... }?

I've never dug into how KtLint was configured, but looking at the Detekt documentation it seems to me that what you did is enough.

The exist a task within the WordPress build.gradle file, called violationCommentsToGitHub. This task has a violations configuration for CheckStyle. Do I need to add something similar for Detekt?

Yes, that task is called on CI and it adds a comment with the violations to the GH PR. It looks like Detekt is supported as well. I think that if we use the default output configuration for the Detekt report, it should pick it up automatically.

ParaskP7 · 2021-02-10T15:13:19Z

👋 @loremattei !

I tested following the steps you provided and it worked as expected, other than what @malinajirka already noted.

Thank you so much for the review! 🙏 Yeap, @malinajirka was really thorough! ❤️

I've never dug into how KtLint was configured, but looking at the Detekt documentation it seems to me that what you did is enough.

Thanks for double checking on that, I agree! 🙏

Yes, that task is called on CI and it adds a comment with the violations to the GH PR. It looks like Detekt is supported as well. I think that if we use the default output configuration for the Detekt report, it should pick it up automatically.

Coolio, let me check on that as well before merging that to develop. Or, question (❓), do you think we can merge it even without this violations configuration and add that one as a separate task afterwards?

ParaskP7 · 2021-02-10T15:38:55Z

👋 @loremattei !

Coolio, let me check on that as well before merging that to develop.

I checked the tool (violation-comments-to-github-gradle-plugin) and maybe the below configuration is what we need to have violation comments work for both Checkstyle and Detekt now. Wdyt?

I'll try to push a commit with that configuration with both, some CheckStyle and/or Detekt issues, to see this violation comments setup work as expected.

…id into detekt-integration

This is done in order to give space to another violation comments configuration, specific to 'Detekt', since the '.xml' path is currently too generic.

This will enable violation comments for 'Detekt'.

ParaskP7 · 2021-02-11T09:19:57Z

👋 @loremattei !

I have been trying to update the violations configuration while having some issues with Detekt (7 of them), but I am not seeing any output. I am not familiar with this tool and how it outputs the violations. Can you maybe help with that?

PS: I am now going to create a separate test branch with a CheckStyle violation, then open a test PR and observe the output (if any). Will keep you posted. #14033
PSS: I also checked Circle CI and the Violations job and saw this below, maybe something is wrong, I am not sure:

#!/bin/bash -eo pipefail
if [ -n "$GITHUB_API_TOKEN" ]; then
  ./gradlew --stacktrace violationCommentsToGitHub -DGITHUB_PULLREQUESTID=${CIRCLE_PULL_REQUEST##*/} -DGITHUB_OAUTH2TOKEN=$GITHUB_API_TOKEN
else
  echo "Not posting lint errors to Github because \$GITHUB_API_TOKEN is not found"
fi

…id into detekt-integration

After testing the 'Violation Comments' plugin it seems that the plugin is not working as expected. As thus, any additional configuration is being reverted. This additional configuration will be added back again when the plugin gets back to work a working state. Link: #14051

Seven new issues were added to the baseline.

ParaskP7 added 4 commits February 8, 2021 16:47

Docs: Add detekt to coding style documentation

5c5b731

ParaskP7 added Tooling [Status] Not Ready for Merge labels Feb 8, 2021

ParaskP7 added this to the 16.8 milestone Feb 8, 2021

ParaskP7 requested review from malinajirka and loremattei February 8, 2021 15:16

ParaskP7 self-assigned this Feb 8, 2021

ParaskP7 changed the title ~~Detekt integration~~ Detekt Integration Feb 8, 2021

malinajirka reviewed Feb 9, 2021

View reviewed changes

This was referenced Feb 9, 2021

Detekt Integration - Phase.2 #14011

Closed

Detekt Integration - Phase.3 #14012

Closed

ParaskP7 changed the title ~~Detekt Integration~~ Detekt Integration - Phase.1 Feb 9, 2021

ParaskP7 added 6 commits February 9, 2021 14:12

Build: Disable auto correct within detekt's yml formatting configuration

68e24e9

This is done in order to align this configuration with the more global root level 'build.gradle' detekt 'autoCorrect = false' configuration.

Build: Add enums as an exception to the magic number rule

56fdf26

Also, note that the baseline was update to exclude these enum related magic numbers.

Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Andro…

4f5bee6

…id into detekt-integration

Build: Update baseline after merging this branch with develop

da11d17

Two new issues were added to the baseline.

ParaskP7 requested a review from malinajirka February 9, 2021 13:33

malinajirka approved these changes Feb 10, 2021

View reviewed changes

loremattei approved these changes Feb 10, 2021

View reviewed changes

ParaskP7 added 4 commits February 10, 2021 17:42

Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Andro…

70467ec

…id into detekt-integration

Build: Make checkstyle.xml file path explicitly for the violation config

14b91ab

This is done in order to give space to another violation comments configuration, specific to 'Detekt', since the '.xml' path is currently too generic.

Build: Add detekt.xml file path to violation config

ceb3deb

This will enable violation comments for 'Detekt'.

Build: Make file path regex for the violation config to match folders

ebfb636

ParaskP7 mentioned this pull request Feb 11, 2021

Test PR (DO NOT MERGE) #14033

Closed

ParaskP7 added 3 commits February 12, 2021 12:42

Merge branch 'develop' of github.com:wordpress-mobile/WordPress-Andro…

ac38ada

…id into detekt-integration

Build: Update baseline after merging this branch with develop

07c352a

Seven new issues were added to the baseline.

ParaskP7 merged commit 1d9e968 into develop Feb 12, 2021

ParaskP7 deleted the detekt-integration branch February 12, 2021 11:32

mzorz mentioned this pull request Feb 15, 2021

Make loadGradleProperties method static so it can be used in different rootProjects Automattic/stories-android#640

Merged

ParaskP7 mentioned this pull request Feb 18, 2021

Detekt Integration - Showcase #13290

Closed

oguzkocer removed the [Status] Not Ready for Merge label Mar 16, 2021

ParaskP7 mentioned this pull request Jul 12, 2022

Update Detekt Configuration/Version #16892

Closed

11 tasks

ParaskP7 mentioned this pull request Aug 2, 2022

Add Detekt wordpress-mobile/WordPress-FluxC-Android#2476

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detekt Integration - Phase.1 #13996

Detekt Integration - Phase.1 #13996

ParaskP7 commented Feb 8, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Feb 8, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Feb 8, 2021 •

edited

Loading

malinajirka left a comment

malinajirka Feb 9, 2021

ParaskP7 Feb 9, 2021

loremattei Feb 10, 2021

ParaskP7 Feb 10, 2021 •

edited

Loading

malinajirka Feb 11, 2021

ParaskP7 commented Feb 9, 2021

ParaskP7 commented Feb 9, 2021 •

edited

Loading

malinajirka left a comment

loremattei left a comment

ParaskP7 commented Feb 10, 2021

ParaskP7 commented Feb 10, 2021 •

edited

Loading

ParaskP7 commented Feb 11, 2021 •

edited

Loading

Detekt Integration - Phase.1 #13996

Detekt Integration - Phase.1 #13996

Conversation

ParaskP7 commented Feb 8, 2021 • edited Loading

peril-wordpress-mobile bot commented Feb 8, 2021 • edited Loading

peril-wordpress-mobile bot commented Feb 8, 2021 • edited Loading

malinajirka left a comment

Choose a reason for hiding this comment

malinajirka Feb 9, 2021

Choose a reason for hiding this comment

ParaskP7 Feb 9, 2021

Choose a reason for hiding this comment

loremattei Feb 10, 2021

Choose a reason for hiding this comment

ParaskP7 Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

malinajirka Feb 11, 2021

Choose a reason for hiding this comment

ParaskP7 commented Feb 9, 2021

ParaskP7 commented Feb 9, 2021 • edited Loading

malinajirka left a comment

Choose a reason for hiding this comment

loremattei left a comment

Choose a reason for hiding this comment

ParaskP7 commented Feb 10, 2021

ParaskP7 commented Feb 10, 2021 • edited Loading

ParaskP7 commented Feb 11, 2021 • edited Loading

ParaskP7 commented Feb 8, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Feb 8, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Feb 8, 2021 •

edited

Loading

ParaskP7 Feb 10, 2021 •

edited

Loading

ParaskP7 commented Feb 9, 2021 •

edited

Loading

ParaskP7 commented Feb 10, 2021 •

edited

Loading

ParaskP7 commented Feb 11, 2021 •

edited

Loading