From eb141e9a4de24cd470098e3689ae3acc1eaf55e5 Mon Sep 17 00:00:00 2001 From: marschwar Date: Fri, 6 Aug 2021 17:54:14 +0200 Subject: [PATCH] Enable more rules from naming rule set for detekt code base (#3996) * Enable more naming rules for detekt code base * Fix NoNameShowing violations * Exclude buildSrc and kotlin scripts from InvalidPackageDeclaration * Improve fix of NoNameShadowing Co-authored-by: Markus Schwarz --- config/detekt/detekt.yml | 13 ++++++++++++ .../detekt/core/config/YamlConfig.kt | 4 ++-- .../detekt/internal/DetektAndroid.kt | 20 +++++++++++-------- .../detekt/internal/DetektMultiplatform.kt | 16 ++++++++------- .../detekt/rules/naming/ForbiddenClassName.kt | 17 ++++++---------- .../rules/naming/ForbiddenClassNameSpec.kt | 11 +++++++++- 6 files changed, 52 insertions(+), 29 deletions(-) diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index 2c6153c55..737b3c321 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -100,6 +100,19 @@ formatting: SpacingBetweenDeclarationsWithComments: active: true +naming: + InvalidPackageDeclaration: + active: true + excludes: ['**/buildSrc/**/*.kt', '**/*.kts'] + NoNameShadowing: + active: true + NonBooleanPropertyPrefixedWithIs: + active: true + VariableMaxLength: + active: true + VariableMinLength: + active: true + potential-bugs: UnsafeCast: active: true diff --git a/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/config/YamlConfig.kt b/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/config/YamlConfig.kt index 06537c797..2f2827b6b 100644 --- a/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/config/YamlConfig.kt +++ b/detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/config/YamlConfig.kt @@ -68,10 +68,10 @@ class YamlConfig internal constructor( * * Note the reader will be consumed and closed. */ - fun load(reader: Reader): Config = reader.buffered().use { + fun load(reader: Reader): Config = reader.buffered().use { bufferedReader -> val map: Map<*, *>? = runCatching { @Suppress("USELESS_CAST") // runtime inference bug - Yaml().loadAs(it, Map::class.java) as Map<*, *>? + Yaml().loadAs(bufferedReader, Map::class.java) as Map<*, *>? }.getOrElse { throw Config.InvalidConfigurationError(it) } if (map == null) { YamlConfig(emptyMap()) diff --git a/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektAndroid.kt b/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektAndroid.kt index abe8ab036..c6ab047e7 100644 --- a/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektAndroid.kt +++ b/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektAndroid.kt @@ -67,28 +67,32 @@ internal class DetektAndroid(private val project: Project) { fun registerTasks(extension: DetektExtension) { // There is not a single Android plugin, but each registers an extension based on BaseExtension, // so we catch them all by looking for this one - project.afterEvaluate { - val baseExtension = project.extensions.findByType(BaseExtension::class.java) - baseExtension?.let { - val bootClasspath = project.files(baseExtension.bootClasspath) + project.afterEvaluate { evaluatedProject -> + val baseExtensionOrNull = evaluatedProject.extensions.findByType(BaseExtension::class.java) + baseExtensionOrNull?.let { baseExtension -> + val bootClasspath = evaluatedProject.files(baseExtension.bootClasspath) baseExtension.variants ?.matching { !extension.matchesIgnoredConfiguration(it) } ?.all { variant -> - project.registerAndroidDetektTask(bootClasspath, extension, variant).also { provider -> + evaluatedProject.registerAndroidDetektTask(bootClasspath, extension, variant).also { provider -> mainTaskProvider.dependsOn(provider) } - project.registerAndroidCreateBaselineTask(bootClasspath, extension, variant) + evaluatedProject.registerAndroidCreateBaselineTask(bootClasspath, extension, variant) .also { provider -> mainBaselineTaskProvider.dependsOn(provider) } variant.testVariants .filter { !extension.matchesIgnoredConfiguration(it) } .forEach { testVariant -> - project.registerAndroidDetektTask(bootClasspath, extension, testVariant) + evaluatedProject.registerAndroidDetektTask(bootClasspath, extension, testVariant) .also { provider -> testTaskProvider.dependsOn(provider) } - project.registerAndroidCreateBaselineTask(bootClasspath, extension, testVariant) + evaluatedProject.registerAndroidCreateBaselineTask( + bootClasspath, + extension, + testVariant + ) .also { provider -> testBaselineTaskProvider.dependsOn(provider) } diff --git a/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektMultiplatform.kt b/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektMultiplatform.kt index 9e70c7405..419a3e695 100644 --- a/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektMultiplatform.kt +++ b/detekt-gradle-plugin/src/main/kotlin/io/gitlab/arturbosch/detekt/internal/DetektMultiplatform.kt @@ -25,8 +25,8 @@ internal class DetektMultiplatform(private val project: Project) { private fun Project.registerMultiplatformTasks(extension: DetektExtension) { // We need another project.afterEvaluate as the Android target is attached on // a project.afterEvaluate inside AGP. We should further investigate and potentially remove this. - project.afterEvaluate { - val kmpExtension = project.extensions.getByType(KotlinMultiplatformExtension::class.java) + project.afterEvaluate { evaluatedProject -> + val kmpExtension = evaluatedProject.extensions.getByType(KotlinMultiplatformExtension::class.java) kmpExtension.targets.all { target -> target.compilations.forEach { compilation -> @@ -36,9 +36,9 @@ internal class DetektMultiplatform(private val project: Project) { val inputSource = compilation.kotlinSourceSets .map { it.kotlin.sourceDirectories } - .fold(project.files() as FileCollection) { collection, next -> collection.plus(next) } + .fold(evaluatedProject.files() as FileCollection) { collection, next -> collection.plus(next) } - val detektTaskProvider = project.registerDetektTask( + val detektTaskProvider = evaluatedProject.registerDetektTask( DetektPlugin.DETEKT_TASK_NAME + taskSuffix, extension ) { @@ -53,7 +53,7 @@ internal class DetektMultiplatform(private val project: Project) { } else { extension.baseline?.takeIf { it.exists() } }?.let { baselineFile -> - baseline.set(layout.file(project.provider { baselineFile })) + baseline.set(layout.file(evaluatedProject.provider { baselineFile })) } reports = extension.reports reports.xml.setDefaultIfUnset(File(extension.reportsDir, compilation.name + ".xml")) @@ -69,7 +69,9 @@ internal class DetektMultiplatform(private val project: Project) { tasks.matching { it.name == LifecycleBasePlugin.CHECK_TASK_NAME } .configureEach { it.dependsOn(detektTaskProvider) } - project.registerCreateBaselineTask(DetektPlugin.BASELINE_TASK_NAME + taskSuffix, extension) { + evaluatedProject.registerCreateBaselineTask( + DetektPlugin.BASELINE_TASK_NAME + taskSuffix, extension + ) { setSource(inputSource) if (runWithTypeResolution) { classpath.setFrom(inputSource, compilation.compileDependencyFiles) @@ -79,7 +81,7 @@ internal class DetektMultiplatform(private val project: Project) { } else { extension.baseline } - baseline.set(project.layout.file(project.provider { variantBaselineFile })) + baseline.set(evaluatedProject.layout.file(evaluatedProject.provider { variantBaselineFile })) description = "Creates detekt baseline for ${target.name} and source set ${compilation.name}" if (runWithTypeResolution) { diff --git a/detekt-rules-naming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ForbiddenClassName.kt b/detekt-rules-naming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ForbiddenClassName.kt index adf31237e..94e8ce811 100644 --- a/detekt-rules-naming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ForbiddenClassName.kt +++ b/detekt-rules-naming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ForbiddenClassName.kt @@ -31,19 +31,14 @@ class ForbiddenClassName(config: Config = Config.empty) : Rule(config) { } override fun visitClassOrObject(classOrObject: KtClassOrObject) { - val name = classOrObject.name - val forbiddenEntries = name?.let { forbiddenName.filter { name.contains(it, ignoreCase = true) } } + val name = classOrObject.name ?: return + val forbiddenEntries = forbiddenName.filter { name.contains(it, ignoreCase = true) } - if (forbiddenEntries?.isNotEmpty() == true) { - var message = "Class name $name is forbidden as it contains:" - forbiddenEntries.forEach { message += " $it," } - message.trimEnd { it == ',' } - - report(CodeSmell(issue, Entity.atName(classOrObject), message)) + if (forbiddenEntries.isEmpty()) { + return } - } - companion object { - const val FORBIDDEN_NAME = "forbiddenName" + val message = "Class name $name is forbidden as it contains: ${forbiddenEntries.joinToString(", ")}" + report(CodeSmell(issue, Entity.atName(classOrObject), message)) } } diff --git a/detekt-rules-naming/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ForbiddenClassNameSpec.kt b/detekt-rules-naming/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ForbiddenClassNameSpec.kt index 4de7ba08c..d8cff7a05 100644 --- a/detekt-rules-naming/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ForbiddenClassNameSpec.kt +++ b/detekt-rules-naming/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ForbiddenClassNameSpec.kt @@ -1,12 +1,13 @@ package io.gitlab.arturbosch.detekt.rules.naming -import io.gitlab.arturbosch.detekt.rules.naming.ForbiddenClassName.Companion.FORBIDDEN_NAME import io.gitlab.arturbosch.detekt.test.TestConfig import io.gitlab.arturbosch.detekt.test.compileAndLint import org.assertj.core.api.Assertions.assertThat import org.spekframework.spek2.Spek import org.spekframework.spek2.style.specification.describe +private const val FORBIDDEN_NAME = "forbiddenName" + class ForbiddenClassNameSpec : Spek({ describe("ForbiddenClassName rule") { @@ -55,5 +56,13 @@ class ForbiddenClassNameSpec : Spek({ ) .hasSize(2) } + it("should report all forbidden names in message") { + val code = """ + class TestManager {}""" + val actual = ForbiddenClassName(TestConfig(mapOf(FORBIDDEN_NAME to "Test, Manager, Provider"))) + .compileAndLint(code) + assertThat(actual.first().message) + .isEqualTo("Class name TestManager is forbidden as it contains: Test, Manager") + } } })