From b7e426832786c84219f719b67fb877f6b31c8dff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Brais=20Gab=C3=ADn?= Date: Mon, 18 Jul 2022 14:24:46 +0200 Subject: [PATCH] Add option to add a reason to `ForbiddenImport` (#4909) * Improve tests * Improve issue message * Add utility to help testing ValueWithReason configurations * Add support for reasons in ForbiddenImport * Improve non-compilant * Update detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImportSpec.kt Co-authored-by: marschwar * data class Co-authored-by: marschwar --- .../detekt/rules/style/ForbiddenImport.kt | 35 +++++++++----- .../detekt/rules/style/ForbiddenImportSpec.kt | 46 ++++++++++++++----- detekt-test/api/detekt-test.api | 4 ++ .../arturbosch/detekt/test/TestConfig.kt | 5 ++ 4 files changed, 66 insertions(+), 24 deletions(-) diff --git a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImport.kt b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImport.kt index 069c1e65fa4..c8ed4d005f4 100644 --- a/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImport.kt +++ b/detekt-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImport.kt @@ -10,6 +10,7 @@ import io.gitlab.arturbosch.detekt.api.Severity import io.gitlab.arturbosch.detekt.api.config import io.gitlab.arturbosch.detekt.api.internal.Configuration import io.gitlab.arturbosch.detekt.api.simplePatternToRegex +import io.gitlab.arturbosch.detekt.api.valuesWithReason import org.jetbrains.kotlin.psi.KtImportDirective /** @@ -17,7 +18,6 @@ import org.jetbrains.kotlin.psi.KtImportDirective * or deprecated APIs. Detekt will then report all imports that are forbidden. * * - * package foo * import kotlin.jvm.JvmField * import kotlin.SinceKotlin * @@ -33,8 +33,8 @@ class ForbiddenImport(config: Config = Config.empty) : Rule(config) { ) @Configuration("imports which should not be used") - private val imports: List by config(emptyList()) { - it.distinct().map(String::simplePatternToRegex) + private val imports: List by config(valuesWithReason()) { list -> + list.map { Forbidden(it.value.simplePatternToRegex(), it.reason) } } @Configuration("reports imports which match the specified regular expression. For example `net.*R`.") @@ -44,18 +44,29 @@ class ForbiddenImport(config: Config = Config.empty) : Rule(config) { super.visitImportDirective(importDirective) val import = importDirective.importedFqName?.asString().orEmpty() - if (imports.any { it.matches(import) } || containsForbiddenPattern(import)) { - report( - CodeSmell( - issue, - Entity.from(importDirective), - "The import " + - "$import has been forbidden in the Detekt config." - ) - ) + + val forbidden = imports.find { it.import.matches(import) } + val reason = if (forbidden != null) { + if (forbidden.reason != null) { + "The import `$import` has been forbidden: ${forbidden.reason}" + } else { + defaultReason(import) + } + } else { + if (containsForbiddenPattern(import)) defaultReason(import) else null + } + + if (reason != null) { + report(CodeSmell(issue, Entity.from(importDirective), reason)) } } + private fun defaultReason(forbiddenImport: String): String { + return "The import `$forbiddenImport` has been forbidden in the Detekt config." + } + private fun containsForbiddenPattern(import: String): Boolean = forbiddenPatterns.pattern.isNotEmpty() && forbiddenPatterns.containsMatchIn(import) } + +private data class Forbidden(val import: Regex, val reason: String?) diff --git a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImportSpec.kt b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImportSpec.kt index 4e89ca8ef30..f092167a31f 100644 --- a/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImportSpec.kt +++ b/detekt-rules-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenImportSpec.kt @@ -1,7 +1,9 @@ package io.gitlab.arturbosch.detekt.rules.style +import io.gitlab.arturbosch.detekt.api.ValueWithReason import io.gitlab.arturbosch.detekt.test.TestConfig import io.gitlab.arturbosch.detekt.test.lint +import io.gitlab.arturbosch.detekt.test.toConfig import org.assertj.core.api.Assertions.assertThat import org.junit.jupiter.api.DisplayName import org.junit.jupiter.api.Test @@ -29,38 +31,54 @@ class ForbiddenImportSpec { @Test fun `should report nothing when imports are blank`() { - val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to " "))).lint(code) + val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf(" ")))).lint(code) assertThat(findings).isEmpty() } @Test fun `should report nothing when imports do not match`() { - val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to "org.*"))).lint(code) + val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("org.*")))).lint(code) assertThat(findings).isEmpty() } @Test @DisplayName("should report kotlin.* when imports are kotlin.*") fun reportKotlinWildcardImports() { - val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to "kotlin.*"))).lint(code) + val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("kotlin.*")))).lint(code) + assertThat(findings) + .extracting("message") + .containsExactlyInAnyOrder( + "The import `kotlin.jvm.JvmField` has been forbidden in the Detekt config.", + "The import `kotlin.SinceKotlin` has been forbidden in the Detekt config.", + ) + } + + @Test + @DisplayName("should report kotlin.* when imports are kotlin.* with reasons") + fun reportKotlinWildcardImports2() { + val config = TestConfig(mapOf(IMPORTS to listOf(ValueWithReason("kotlin.*", "I'm just joking!").toConfig()))) + val findings = ForbiddenImport(config).lint(code) assertThat(findings).hasSize(2) + assertThat(findings[0].message) + .isEqualTo("The import `kotlin.jvm.JvmField` has been forbidden: I'm just joking!") + assertThat(findings[1].message) + .isEqualTo("The import `kotlin.SinceKotlin` has been forbidden: I'm just joking!") } @Test @DisplayName("should report kotlin.SinceKotlin when specified via fully qualified name") fun reportKotlinSinceKotlinWhenFqdnSpecified() { - val findings = - ForbiddenImport(TestConfig(mapOf(IMPORTS to "kotlin.SinceKotlin"))).lint(code) - assertThat(findings).hasSize(1) + val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("kotlin.SinceKotlin")))).lint(code) + assertThat(findings) + .hasSize(1) } @Test @DisplayName("should report kotlin.SinceKotlin and kotlin.jvm.JvmField when specified via fully qualified names") fun reportMultipleConfiguredImportsCommaSeparated() { val findings = - ForbiddenImport(TestConfig(mapOf(IMPORTS to "kotlin.SinceKotlin,kotlin.jvm.JvmField"))).lint( - code - ) + ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("kotlin.SinceKotlin", "kotlin.jvm.JvmField")))) + .lint(code) assertThat(findings).hasSize(2) } @@ -81,14 +99,14 @@ class ForbiddenImportSpec { @Test @DisplayName("should report kotlin.SinceKotlin when specified via kotlin.Since*") fun reportsKotlinSinceKotlinWhenSpecifiedWithWildcard() { - val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to "kotlin.Since*"))).lint(code) + val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("kotlin.Since*")))).lint(code) assertThat(findings).hasSize(1) } @Test @DisplayName("should report all of com.example.R.string, net.example.R.dimen, and net.example.R.dimension") fun preAndPostWildcard() { - val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to "*.R.*"))).lint(code) + val findings = ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("*.R.*")))).lint(code) assertThat(findings).hasSize(3) } @@ -96,7 +114,7 @@ class ForbiddenImportSpec { @DisplayName("should report net.example.R.dimen but not net.example.R.dimension") fun doNotReportSubstringOfFqdn() { val findings = - ForbiddenImport(TestConfig(mapOf(IMPORTS to "net.example.R.dimen"))).lint(code) + ForbiddenImport(TestConfig(mapOf(IMPORTS to listOf("net.example.R.dimen")))).lint(code) assertThat(findings).hasSize(1) } @@ -112,5 +130,9 @@ class ForbiddenImportSpec { val findings = ForbiddenImport(TestConfig(mapOf(FORBIDDEN_PATTERNS to "net.*R|com.*expiremental"))).lint(code) assertThat(findings).hasSize(2) + assertThat(findings[0].message) + .isEqualTo("The import `net.example.R.dimen` has been forbidden in the Detekt config.") + assertThat(findings[1].message) + .isEqualTo("The import `net.example.R.dimension` has been forbidden in the Detekt config.") } } diff --git a/detekt-test/api/detekt-test.api b/detekt-test/api/detekt-test.api index 3eaa3c7710d..bec57a6dbe7 100644 --- a/detekt-test/api/detekt-test.api +++ b/detekt-test/api/detekt-test.api @@ -55,6 +55,10 @@ public final class io/gitlab/arturbosch/detekt/test/TestConfig$Companion { public final fun invoke ([Lkotlin/Pair;)Lio/gitlab/arturbosch/detekt/test/TestConfig; } +public final class io/gitlab/arturbosch/detekt/test/TestConfigKt { + public static final fun toConfig (Lio/gitlab/arturbosch/detekt/api/ValueWithReason;)Ljava/util/Map; +} + public final class io/gitlab/arturbosch/detekt/test/ThresholdedCodeSmellAssert : org/assertj/core/api/AbstractAssert { public fun (Lio/gitlab/arturbosch/detekt/api/ThresholdedCodeSmell;)V public final fun hasThreshold (I)V diff --git a/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/TestConfig.kt b/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/TestConfig.kt index a84ad05e2c4..7659bec664d 100644 --- a/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/TestConfig.kt +++ b/detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/TestConfig.kt @@ -1,6 +1,7 @@ package io.gitlab.arturbosch.detekt.test import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.ValueWithReason import io.gitlab.arturbosch.detekt.core.config.tryParseBasedOnDefault import io.gitlab.arturbosch.detekt.core.config.valueOrDefaultInternal @@ -53,3 +54,7 @@ open class TestConfig( operator fun invoke(vararg pairs: Pair) = TestConfig(mapOf(*pairs)) } } + +fun ValueWithReason.toConfig(): Map { + return mapOf("value" to value, "reason" to reason) +}