Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ignoreOverridden support for BooleanPropertyNaming rule #4654

Merged
merged 4 commits into from Mar 31, 2022
Merged

Add ignoreOverridden support for BooleanPropertyNaming rule #4654

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e7a5566
Add ignoreOverridden support for BooleanPropertyNaming
mhernand40 Mar 29, 2022
4ccba22
Default to true for ignoreOverridden
mhernand40 Mar 30, 2022
4d54a66
Fix tests
mhernand40 Mar 30, 2022
369cdda
Update default-detekt-config.yml
mhernand40 Mar 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
View file
Open in desktop
Expand Up @@ -284,6 +284,7 @@ naming:
BooleanPropertyNaming:
active: false
allowedPattern: '^(is|has|are)'
ignoreOverridden: false
ClassNaming:
active: true
classPattern: '[A-Z][a-zA-Z0-9]*'
Expand Down
8 changes: 7 additions & 1 deletion ...-naming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/BooleanPropertyNaming.kt
View file
Open in desktop
Expand Up @@ -12,6 +12,7 @@ import io.gitlab.arturbosch.detekt.api.internal.Configuration
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import io.gitlab.arturbosch.detekt.rules.fqNameOrNull
import io.gitlab.arturbosch.detekt.rules.identifierName
import io.gitlab.arturbosch.detekt.rules.isOverride
import org.jetbrains.kotlin.psi.KtCallableDeclaration
import org.jetbrains.kotlin.psi.KtParameter
import org.jetbrains.kotlin.psi.KtProperty
Expand All @@ -35,6 +36,9 @@ class BooleanPropertyNaming(config: Config = Config.empty) : Rule(config) {
@Configuration("naming pattern")
private val allowedPattern: Regex by config("^(is|has|are)", String::toRegex)

@Configuration("ignores properties that have the override modifier")
private val ignoreOverridden: Boolean by config(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would default this to true. I know, this is to avoid behaviour change but for me this is more a bug fix than a "behaviour change".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to address and make that change. Would others care to weigh in what their thoughts are before I do so? Thank you 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, this is reasonable to be defaulted to true. Is a "bug-fix on steroids"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you so much. 🙂


override val issue = Issue(
javaClass.simpleName, Severity.CodeSmell,
"Boolean property name should follow the naming convention set in the projects configuration.",
Expand Down Expand Up @@ -65,7 +69,7 @@ class BooleanPropertyNaming(config: Config = Config.empty) : Rule(config) {
val isBooleanType =
typeName == KOTLIN_BOOLEAN_TYPE_NAME || typeName == JAVA_BOOLEAN_TYPE_NAME

if (isBooleanType && !name.contains(allowedPattern)) {
if (isBooleanType && !name.contains(allowedPattern) && !isIgnoreOverridden(declaration)) {
report(reportCodeSmell(declaration, name))
}
}
Expand All @@ -89,6 +93,8 @@ class BooleanPropertyNaming(config: Config = Config.empty) : Rule(config) {
.toString()
}

private fun isIgnoreOverridden(declaration: KtCallableDeclaration) = ignoreOverridden && declaration.isOverride()

companion object {
const val KOTLIN_BOOLEAN_TYPE_NAME = "kotlin.Boolean"
const val JAVA_BOOLEAN_TYPE_NAME = "java.lang.Boolean"
Expand Down
282 changes: 282 additions & 0 deletions ...ing/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/naming/BooleanPropertyNamingSpec.kt
View file
Open in desktop
Expand Up @@ -26,6 +26,35 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean override`() {
val code = """
interface Test {
val default: Boolean
}

data class TestImpl (override var default: Boolean) : Test
"""
val findings = subject.compileAndLintWithContext(env, code)

assertThat(findings).hasSize(2)
}

@Test
fun `should not warn about Kotlin Boolean override if ignoreOverridden is true`() {
val code = """
interface Test {
val default: Boolean
}

data class TestImpl (override var default: Boolean) : Test
"""
val config = TestConfig(mapOf(IGNORE_OVERRIDDEN to true))
val findings = BooleanPropertyNaming(config).compileAndLintWithContext(env, code)

assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean nullable`() {
val code = """data class Test (var default: Boolean?)"""
Expand All @@ -34,6 +63,35 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean nullable override`() {
val code = """
interface Test {
val default: Boolean?
}

data class TestImpl (override var default: Boolean?) : Test
"""
val findings = subject.compileAndLintWithContext(env, code)

assertThat(findings).hasSize(2)
}

@Test
fun `should not warn about Kotlin Boolean nullable override if ignoreOverridden is true`() {
val code = """
interface Test {
val default: Boolean?
}

data class TestImpl (override var default: Boolean?) : Test
"""
val config = TestConfig(mapOf(IGNORE_OVERRIDDEN to true))
val findings = BooleanPropertyNaming(config).compileAndLintWithContext(env, code)

assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean initialized`() {
val code = """data class Test (var default: Boolean = false)"""
Expand All @@ -42,6 +100,35 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean initialized override`() {
val code = """
interface Test {
val default: Boolean
}

data class TestImpl (override var default: Boolean = false) : Test
"""
val findings = subject.compileAndLintWithContext(env, code)

assertThat(findings).hasSize(2)
}

@Test
fun `should not warn about Kotlin Boolean initialized override if ignoreOverridden is true`() {
val code = """
interface Test {
val default: Boolean
}

data class TestImpl (override var default: Boolean = false) : Test
"""
val config = TestConfig(mapOf(IGNORE_OVERRIDDEN to true))
val findings = BooleanPropertyNaming(config).compileAndLintWithContext(env, code)

assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Java Boolean`() {
val code = """data class Test (var default: java.lang.Boolean)"""
Expand All @@ -50,6 +137,35 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Java Boolean override`() {
val code = """
interface Test {
val default: java.lang.Boolean
}

data class TestImpl (override var default: java.lang.Boolean) : Test
"""
val findings = subject.compileAndLintWithContext(env, code)

assertThat(findings).hasSize(2)
}

@Test
fun `should not warn about Java Boolean override if ignoreOverridden is true`() {
val code = """
interface Test {
val default: java.lang.Boolean
}

data class TestImpl (override var default: java.lang.Boolean) : Test
"""
val config = TestConfig(mapOf(IGNORE_OVERRIDDEN to true))
val findings = BooleanPropertyNaming(config).compileAndLintWithContext(env, code)

assertThat(findings).hasSize(1)
}

@Test
fun `should not detect primitive types`() {
val code = """data class Test (var count: Int)"""
Expand Down Expand Up @@ -81,6 +197,39 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean override`() {
val code = """
interface Test {
val default: Boolean
}

class TestImpl : Test {
override var default: Boolean = true
}
"""
val findings = subject.compileAndLintWithContext(env, code)

assertThat(findings).hasSize(2)
}

@Test
fun `should not warn about Kotlin Boolean override if isIgnoreOverridden is true`() {
val code = """
interface Test {
val default: Boolean
}

class TestImpl : Test {
override var default: Boolean = true
}
"""
val config = TestConfig(mapOf(IGNORE_OVERRIDDEN to true))
val findings = BooleanPropertyNaming(config).compileAndLintWithContext(env, code)

assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean nullable`() {
val code = """
Expand All @@ -93,6 +242,39 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean nullable override`() {
val code = """
interface Test {
val default: Boolean?
}

class TestImpl : Test {
override var default: Boolean? = null
}
"""
val findings = subject.compileAndLintWithContext(env, code)

assertThat(findings).hasSize(2)
}

@Test
fun `should not warn about Kotlin Boolean nullable override if ignoreOverridden is true`() {
val code = """
interface Test {
val default: Boolean?
}

class TestImpl : Test {
override var default: Boolean? = null
}
"""
val config = TestConfig(mapOf(IGNORE_OVERRIDDEN to true))
val findings = BooleanPropertyNaming(config).compileAndLintWithContext(env, code)

assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean initialized`() {
val code = """
Expand All @@ -105,6 +287,39 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Kotlin Boolean initialized override`() {
val code = """
interface Test {
val default: Boolean
}

class TestImpl : Test {
override var default: Boolean = false
}
"""
val findings = subject.compileAndLintWithContext(env, code)

assertThat(findings).hasSize(2)
}

@Test
fun `should not warn about Kotlin Boolean initialized override if ignoreOverridden is true`() {
val code = """
interface Test {
val default: Boolean
}

class TestImpl : Test {
override var default: Boolean = false
}
"""
val config = TestConfig(mapOf(IGNORE_OVERRIDDEN to true))
val findings = BooleanPropertyNaming(config).compileAndLintWithContext(env, code)

assertThat(findings).hasSize(1)
}

@Test
fun `should warn about inferred boolean type`() {
val code = """
Expand All @@ -117,6 +332,39 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).hasSize(1)
}

@Test
fun `should warn about inferred boolean type override`() {
val code = """
interface Test {
val default: Boolean
}

class TestImpl : Test {
override var default = true
}
"""
val findings = subject.compileAndLintWithContext(env, code)

assertThat(findings).hasSize(2)
}

@Test
fun `should not warn about inferred boolean type override if ignoreOverridden is true`() {
val code = """
interface Test {
val default: Boolean
}

class TestImpl : Test {
override var default = true
}
"""
val config = TestConfig(mapOf(IGNORE_OVERRIDDEN to true))
val findings = BooleanPropertyNaming(config).compileAndLintWithContext(env, code)

assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Java Boolean`() {
val code = """
Expand All @@ -129,6 +377,39 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
assertThat(findings).hasSize(1)
}

@Test
fun `should warn about Java Boolean override`() {
val code = """
interface Test {
val default: java.lang.Boolean
}

class TestImpl : Test {
override var default: java.lang.Boolean = java.lang.Boolean(true)
}
"""
val findings = subject.compileAndLintWithContext(env, code)

assertThat(findings).hasSize(2)
}

@Test
fun `should not warn about Java Boolean override if ignoreOverridden is true`() {
val code = """
interface Test {
val default: java.lang.Boolean
}

class TestImpl : Test {
override var default: java.lang.Boolean = java.lang.Boolean(true)
}
"""
val config = TestConfig(mapOf(IGNORE_OVERRIDDEN to true))
val findings = BooleanPropertyNaming(config).compileAndLintWithContext(env, code)

assertThat(findings).hasSize(1)
}

@Test
fun `should not detect primitive types`() {
val code = """
Expand Down Expand Up @@ -171,3 +452,4 @@ class BooleanPropertyNamingSpec(val env: KotlinCoreEnvironment) {
}

private const val ALLOWED_PATTERN = "allowedPattern"
private const val IGNORE_OVERRIDDEN = "ignoreOverridden"