Detekt

[bmarty]

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

[github-actions]

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.

[ouchadam]

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/

[bmarty]

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.

[bmarty]

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

[ouchadam]

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

[ouchadam]

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]

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]

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]

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)

[github-actions]

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