Detekt #6038

bmarty · 2022-05-12T15:02:59Z

Let the CI fails first!

Now the CI should not fail.

This PR setup detekt and fix a few issues detected by detekt.

Some rule have been disabled temporarily, to avoid having too many changes.

Change on generated analytics file has been reported in matrix-org/matrix-analytics-events#62

@ouchadam

@ouchadam your test fun names are too long!

github-actions · 2022-05-12T15:42:15Z

Unit Test Results

0 files - 122 0 suites - 122 0s ⏱️ - 2m 8s
0 tests - 205 0 ✔️ - 205 0 💤 ±0 0 ❌ ±0
0 runs - 690 0 ✔️ - 690 0 💤 ±0 0 ❌ ±0

Results for commit a312115. ± Comparison against base commit 3f8ddbe.

This pull request removes 205 tests.

im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ given lateinit user properties when valid analytics id updates then identify with lateinit properties
im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ given user consent when tracking events then submits to posthog
im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ given user consent when tracking screen events then submits to posthog
im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ given user has not consented when tracking events then does not track
im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ given user has not consented when tracking screen events then does not track
im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ when consenting to analytics then updates posthog opt out to false
im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ when revoking consent to analytics then updates posthog opt out to true
im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ when setting the analytics id then updates analytics store
im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ when setting user consent then updates analytics store
im.vector.app.features.analytics.impl.DefaultVectorAnalyticsTest ‑ when signing out then resets posthog
…

♻️ This comment has been updated with latest results.

UserProperties fix is also in matrix-org/matrix-analytics-events#62

ouchadam · 2022-05-13T07:08:12Z

build.gradle

+        // preconfigure defaults
+        buildUponDefaultConfig = true
+        // activate all available (even unstable) rules.
+        allRules = false


we can generate a baseline if we want to enable more rules without having to fix them straight away https://detekt.dev/docs/introduction/baseline/

Yes, thanks, I have seen that. Good to prevent new issues. I would prefer to enable the disabled rules one by one and fix the existing issues though.

I have set this value to true and the output is the same.

ouchadam · 2022-05-13T07:09:27Z

tools/detekt/detekt.yml

+
+performance:
+  SpreadOperator:
+    active: false


I don't mind enabling this for production code but it can be handy to be able to use them in tests

ouchadam · 2022-05-13T07:17:32Z

detekt can be quite quick to run, on previous projects I've included it as part of the prebuild tasks to trigger detekt to run on every compile (makes the feedback loop super quick)

// module build.gradle
preBuild.dependsOn(project.tasks.getByName("detekt"))

bmarty · 2022-05-13T07:26:46Z

Detekt can also analyses documentation issues 🎉 , but this is disabled by default. I was looking for such tools when setting up Dokka!

To add to the file detekt.yml:

comments:
  OutdatedDocumentation:
    active: true
  UndocumentedPublicClass:
    active: true
  UndocumentedPublicFunction:
    active: true
  UndocumentedPublicProperty:
    active: true

I will probably add those rules in the file now, but disabled by default, so that it can be handled in dedicated PRs (there are many issues :/). The top priority one will be OutdatedDocumentation

bmarty · 2022-05-13T07:31:03Z

detekt can be quite quick to run, on previous projects I've included it as part of the prebuild tasks to trigger detekt to run on every compile (makes the feedback loop super quick)
// module build.gradle
preBuild.dependsOn(project.tasks.getByName("detekt"))

It can be painful when developing to be blocked by for instance a temporary unused private fun (even if this specific rule is disabled now).

On the other side, it's painful to not see the error locally, but only after the CI checks.

We could have some check_code_before_creating_pr.sh which will run all that can be run locally and will be anyway run by the CI. I'm thinking about compiling android tests, run ktlint, run detekt, etc. WDYT?

ouchadam · 2022-05-13T07:51:04Z

We could have some check_code_before_creating_pr.sh which will run all that can be run locally and will be anyway run by the CI. I'm thinking about compiling android tests, run ktlint, run detekt, etc. WDYT?

having a script to run all our tools detekt, ktlint and the other code quality script would be helpful
there's already a check that causes the build to fail for unused variables 🤔 (which I agree can be quite annoying during development)

No new defect detected.

github-actions · 2022-05-13T22:11:52Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=15 failures=5 errors=0 skipped=3
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=5 failures=1 errors=0 skipped=0
[org.matrix.android.sdk.ordering]
= passed=16 failures=0 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
= passed=2 failures=0 errors=0 skipped=0

bmarty added 13 commits May 12, 2022 16:41

Set up detekt gradle plugin

c02fb87

Detekt: configure and ignore some rules.

9834371

Let GA run detekt

911bfe2

Ignore rule

7d6c27e

We won't use this

d8cda04

"TODO:" is forbidden by Detekt

824f029

Ignore more rules

c5f9546

Detekt: fix MayBeConst

a09e893

Detekt: fix EmptyDefaultConstructor

cdbc9db

Detekt: fix UtilityClassWithPublicConstructor

c19563b

Detekt: ignore EmptyCatchBlock

2190405

Detekt: fix RethrowCaughtException

aeb6495

Detekt: fix MaxLineLength

3bc84f0

@ouchadam your test fun names are too long!

bmarty and others added 7 commits May 12, 2022 17:44

Detekt: fix MatchingDeclarationName

c3b5ba5

Detekt: fix ArrayPrimitive

f02bad5

Ignore more rules

c303b9a

Detekt: fix FunctionParameterNaming

6f3b9c7

Detekt: fix ConstructorParameterNaming

03ec994

UserProperties fix is also in matrix-org/matrix-analytics-events#62

This is now detected by detekt

b70bc20

Changelog

d17a273

bmarty requested review from a team, yostyle and ganfra and removed request for a team May 12, 2022 20:59

bmarty marked this pull request as ready for review May 12, 2022 21:00

bmarty requested a review from ouchadam May 12, 2022 21:00

ouchadam reviewed May 13, 2022

View reviewed changes

ouchadam approved these changes May 13, 2022

View reviewed changes

bmarty added 2 commits May 13, 2022 09:51

Detekt: add rules about comment

cf3e34e

Activate all available (even unstable) rules

a312115

No new defect detected.

bmarty enabled auto-merge May 13, 2022 15:34

bmarty disabled auto-merge May 13, 2022 21:40

bmarty merged commit 9234c60 into develop May 13, 2022

bmarty deleted the feature/bma/detekt branch May 13, 2022 21:40

bmarty mentioned this pull request May 16, 2022

Try detekt #6037

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detekt #6038

Detekt #6038

bmarty commented May 12, 2022 •

edited

Loading

github-actions bot commented May 12, 2022 •

edited

Loading

ouchadam May 13, 2022 •

edited

Loading

bmarty May 13, 2022

bmarty May 13, 2022

ouchadam May 13, 2022

ouchadam commented May 13, 2022

bmarty commented May 13, 2022 •

edited

Loading

bmarty commented May 13, 2022

ouchadam commented May 13, 2022 •

edited

Loading

github-actions bot commented May 13, 2022

Detekt #6038

Detekt #6038

Conversation

bmarty commented May 12, 2022 • edited Loading

github-actions bot commented May 12, 2022 • edited Loading

Unit Test Results

ouchadam May 13, 2022 • edited Loading

Choose a reason for hiding this comment

bmarty May 13, 2022

Choose a reason for hiding this comment

bmarty May 13, 2022

Choose a reason for hiding this comment

ouchadam May 13, 2022

Choose a reason for hiding this comment

ouchadam commented May 13, 2022

bmarty commented May 13, 2022 • edited Loading

bmarty commented May 13, 2022

ouchadam commented May 13, 2022 • edited Loading

github-actions bot commented May 13, 2022

Matrix SDK

Integration Tests Results:

bmarty commented May 12, 2022 •

edited

Loading

github-actions bot commented May 12, 2022 •

edited

Loading

ouchadam May 13, 2022 •

edited

Loading

bmarty commented May 13, 2022 •

edited

Loading

ouchadam commented May 13, 2022 •

edited

Loading