Add detekt

[Rawa]

This PR adds detekt as a linting to ensure we have nice code quality in our project. We've done some evaluation and have opted not to use it for formatting. There we will keep using ktfmt until we see a clear benefit to switch to something else.

---

This change is 

[linear]

DROID-559 Try replacing ktfmt with Detekt

[albin-mullvad]

Please add a description to the PR where you clarify how we intend to use detekt as of this PR.

Reviewed 20 of 22 files at r1, 1 of 1 files at r5.
Reviewable status: 20 of 24 files reviewed, 3 unresolved discussions (waiting on @Rawa)

---

android/build.gradle.kts line 32 at r2 (raw file):

allprojects {
    apply(plugin = Dependencies.Plugin.dependencyCheckId)
    apply(plugin = Dependencies.Plugin.ktfmtId)

Are we completely removing ktfmt? Wasn't the plan to use it in combination with detekt? 🤔

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt line 144 at r2 (raw file):

            is Inet4Address -> 32
            is Inet6Address -> 128
            else -> throw IllegalArgumentException("Invalid IP address (not IPv4 nor IPv6)")

Just to double check; was this one incorrect before?

Code quote:

IllegalArgumentException

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/endpoint/SettingsListener.kt line 11 at r2 (raw file):

import net.mullvad.mullvadvpn.lib.ipc.Event
import net.mullvad.mullvadvpn.lib.ipc.Request
import net.mullvad.mullvadvpn.model.*

Is there a risk that expanding the imports might affect ktfmt or the android studio formatting?

[Rawa]

Reviewable status: 20 of 24 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt line 144 at r2 (raw file):

Previously, albin-mullvad wrote…

Just to double check; was this one incorrect before?

We got a warning for using a too generic exception. Thus I updated throw an IlleagalArgumentException

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/endpoint/SettingsListener.kt line 11 at r2 (raw file):

Previously, albin-mullvad wrote…

Is there a risk that expanding the imports might affect ktfmt or the android studio formatting?

I tried Android Studio Reformat Code and ktfmtFormat, none complained about expanding them. I believe the default in android studio should also not use wildcard.

[Rawa]

Reviewable status: 17 of 24 files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)

---

android/build.gradle.kts line 32 at r2 (raw file):

Previously, albin-mullvad wrote…

Are we completely removing ktfmt? Wasn't the plan to use it in combination with detekt? 🤔

Issue with Android Studio not showing diffs correctly. Restored now.

[albin-mullvad]

Reviewed 1 of 2 files at r3, 1 of 1 files at r4, 4 of 4 files at r6, all commit messages.
Reviewable status: 23 of 24 files reviewed, 3 unresolved discussions (waiting on @Rawa)

---

-- commits line 1 at r6:
Reminder to clean up the commit history a bit before merging.

---

android/build.gradle.kts line 57 at r6 (raw file):

// Kotlin DSL
tasks.withType<Detekt>().configureEach { jvmTarget = "1.8" }

Is there a reason for not using Versions.jvmTarget? It's set to "17" btw. Same applies below.

Code quote:

jvmTarget = "1.8"

---

android/config/detekt.yml line 1 at r6 (raw file):

build:

Is this file based on some default suggestions? It might be nice to add some info or documentation regarding that to the file as a header. Also, if any deviations are made from the default suggestion, it would be nice clarify using comments.

---

android/config/detekt.yml line 94 at r6 (raw file):

      - '**/androidInstrumentedTest/**'
      - '**/jsTest/**'
      - '**/iosTest/**'

Are these needed? 🤔

This also applies to some more parts of this file.

Code quote:

      - '**/jsTest/**'
      - '**/iosTest/**'

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt line 144 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

We got a warning for using a too generic exception. Thus I updated throw an IlleagalArgumentException

Aha, so RuntimeException is counted as too generic?

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/endpoint/SettingsListener.kt line 11 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

I tried Android Studio Reformat Code and ktfmtFormat, none complained about expanding them. I believe the default in android studio should also not use wildcard.

Alright 👍

[Pururun]

Reviewed 1 of 22 files at r1, 1 of 21 files at r7, 1 of 19 files at r8, 19 of 19 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Rawa)

---

android/build.gradle.kts line 32 at r9 (raw file):

allprojects {
    apply(plugin = Dependencies.Plugin.dependencyCheckId)
    apply(plugin = Dependencies.Plugin.ktfmtId)

Will this mean that we no longer will have formatting done by ktfmt

---

android/build.gradle.kts line 44 at r9 (raw file):

    }

    configure<com.ncorti.ktfmt.gradle.KtfmtExtension> {

See above

[Rawa]

Reviewable status: 3 of 24 files reviewed, 5 unresolved discussions (waiting on @albin-mullvad and @Pururun)

---

android/build.gradle.kts line 57 at r6 (raw file):

Previously, albin-mullvad wrote…

Is there a reason for not using Versions.jvmTarget? It's set to "17" btw. Same applies below.

Thanks, legacy, cleaned up :)

---

android/build.gradle.kts line 32 at r9 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Will this mean that we no longer will have formatting done by ktfmt

Reviewable shows these lines really weird for me, didn't show any change. Now it shows it as an addition. I've tried to restore them now.

---

android/build.gradle.kts line 44 at r9 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

See above

See above

---

android/config/detekt.yml line 1 at r6 (raw file):

Previously, albin-mullvad wrote…

Is this file based on some default suggestions? It might be nice to add some info or documentation regarding that to the file as a header. Also, if any deviations are made from the default suggestion, it would be nice clarify using comments.

Yes, one can generate a basic config from ./gradlew detektGenerateConfig then I've done some corrections to make it play nice with compose etc.

---

android/config/detekt.yml line 94 at r6 (raw file):

Previously, albin-mullvad wrote…

Are these needed? 🤔

This also applies to some more parts of this file.

Not really, but part of the standard generated config. I was thinking less change makes us less prone to have unexpected behavior. Do you think we should remove it?

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt line 144 at r2 (raw file):
Yes, not sure exact logic for that rule but it warned it to be too generic in this case.

> Task :detekt FAILED
/Users/rawa/Code/mullvadvpn-app/android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt:144:21: RuntimeException is a too generic Exception. Prefer throwing specific exceptions that indicate a specific error case. [TooGenericExceptionThrown]```

[Pururun]

Reviewed 1 of 21 files at r10, 1 of 1 files at r11, 19 of 19 files at r12, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @albin-mullvad)

[albin-mullvad]

@Rawa did you see my comment above? ⬆️

Reviewed 1 of 21 files at r10, 1 of 19 files at r12.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)

---

android/config/detekt.yml line 1 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Yes, one can generate a basic config from ./gradlew detektGenerateConfig then I've done some corrections to make it play nice with compose etc.

Can we add the generated config as a separate commit to clarify that it's genereated as well as easily see what we've changed from the default?

---

android/config/detekt.yml line 94 at r6 (raw file):

Previously, Rawa (David Göransson) wrote…

Not really, but part of the standard generated config. I was thinking less change makes us less prone to have unexpected behavior. Do you think we should remove it?

Alright 👍

[albin-mullvad]

Reviewed 1 of 22 files at r1, 1 of 1 files at r11, 18 of 19 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Rawa)

[Rawa]

@albin-mullvad Fixed!

Reviewed 1 of 5 files at r13, all commit messages.
Reviewable status: 19 of 24 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @Pururun)

---

android/config/detekt.yml line 1 at r6 (raw file):

Previously, albin-mullvad wrote…

Can we add the generated config as a separate commit to clarify that it's genereated as well as easily see what we've changed from the default?

I've now split them out to 2 separate commits

[Rawa]

Reviewed 4 of 5 files at r13.
Reviewable status: 19 of 24 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad)

[Rawa]

Reviewed all commit messages.
Reviewable status: 19 of 24 files reviewed, 1 unresolved discussion (waiting on @albin-mullvad and @Rawa)

[albin-mullvad]

Reviewed 5 of 5 files at r13, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[albin-mullvad]

Reviewable status: complete! all files reviewed, all discussions resolved

[albin-mullvad]

Reviewed 2 of 2 files at r15, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved