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

Remove ignoreOverridden config from BooleanPropertyNaming etc. #5718

Merged
merged 12 commits into from
Feb 25, 2023
Merged

Remove ignoreOverridden config from BooleanPropertyNaming etc. #5718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cfa074a
Remove ignoreOverridden config for BooleanPropertyNaming rule
drawers Jan 21, 2023
eab40db
Remove ignoreOverridden config for ConstructorParameterNaming rule
drawers Jan 21, 2023
766c8bc
Remove ignoreOverridden config for FunctionNaming rule
drawers Jan 21, 2023
7610a66
Remove ignoreOverridden config for VariableNaming rule
drawers Jan 21, 2023
a5c4450
Remove ignoreOverridden config for FunctionParameterNaming rule
drawers Jan 21, 2023
dae603f
Add back configurations as deprecated
drawers Feb 24, 2023
d5e5111
Add back mistakenly deleted test
drawers Feb 24, 2023
7212563
Remove senseless reference to "default"
drawers Feb 24, 2023
c6c00e6
Fix broken build
drawers Feb 25, 2023
4394343
Restore tests to version on origin/main to make sure we captured inde…
drawers Feb 25, 2023
c4fac22
Replay change to tests on top of formatted tests
drawers Feb 25, 2023
ba2cc61
Fix broken build
drawers Feb 25, 2023
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
5 changes: 0 additions & 5 deletions detekt-core/src/main/resources/default-detekt-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,6 @@ naming:
BooleanPropertyNaming:
active: false
allowedPattern: '^(is|has|are)'
ignoreOverridden: true
ClassNaming:
active: true
classPattern: '[A-Z][a-zA-Z0-9]*'
Expand All @@ -315,7 +314,6 @@ naming:
parameterPattern: '[a-z][A-Za-z0-9]*'
privateParameterPattern: '[a-z][A-Za-z0-9]*'
excludeClassPattern: '$^'
ignoreOverridden: true
EnumNaming:
active: true
enumEntryPattern: '[A-Z][_a-zA-Z0-9]*'
Expand All @@ -333,12 +331,10 @@ naming:
excludes: ['**/test/**', '**/androidTest/**', '**/commonTest/**', '**/jvmTest/**', '**/androidUnitTest/**', '**/androidInstrumentedTest/**', '**/jsTest/**', '**/iosTest/**']
functionPattern: '[a-z][a-zA-Z0-9]*'
excludeClassPattern: '$^'
ignoreOverridden: true
FunctionParameterNaming:
active: true
parameterPattern: '[a-z][A-Za-z0-9]*'
excludeClassPattern: '$^'
ignoreOverridden: true
InvalidPackageDeclaration:
active: true
rootPackage: ''
Expand Down Expand Up @@ -380,7 +376,6 @@ naming:
variablePattern: '[a-z][A-Za-z0-9]*'
privateVariablePattern: '(_)?[a-z][A-Za-z0-9]*'
excludeClassPattern: '$^'
ignoreOverridden: true

performance:
active: true
Expand Down
5 changes: 5 additions & 0 deletions detekt-core/src/main/resources/deprecation.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,13 @@ potential-bugs>IgnoredReturnValue>restrictToAnnotatedMethods=Use `restrictToConf
potential-bugs>LateinitUsage>excludeAnnotatedProperties=Use `ignoreAnnotated` instead
potential-bugs>MissingWhenCase=Rule deprecated as compiler performs this check by default
potential-bugs>RedundantElseInWhen=Rule deprecated as compiler performs this check by default
naming>BooleanPropertyNaming>ignoreOverridden=This configuration is ignored and will be removed in the future
naming>ConstructorParameterNaming>ignoreOverridden=This configuration is ignored and will be removed in the future
naming>FunctionNaming>ignoreOverridden=This configuration is ignored and will be removed in the future
naming>FunctionParameterNaming>ignoreOverriddenFunctions=Use `ignoreOverridden` instead
naming>FunctionParameterNaming>ignoreOverridden=This configuration is ignored and will be removed in the future
naming>MemberNameEqualsClassName>ignoreOverriddenFunction=Use `ignoreOverridden` instead
naming>VariableNaming>ignoreOverridden=This configuration is ignored and will be removed in the future
style>FunctionOnlyReturningConstant>excludeAnnotatedFunction=Use `ignoreAnnotated` instead
style>UnderscoresInNumericLiterals>acceptableDecimalLength=Use `acceptableLength` instead
style>UnnecessaryAbstractClass>excludeAnnotatedClasses=Use `ignoreAnnotated` instead
Expand Down
6 changes: 3 additions & 3 deletions ...-naming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/BooleanPropertyNaming.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ class BooleanPropertyNaming(config: Config = Config.empty) : Rule(config) {
private val allowedPattern: Regex by config("^(is|has|are)", String::toRegex)

@Configuration("ignores properties that have the override modifier")
@Deprecated("This configuration is ignored and will be removed in the future")
@Suppress("UnusedPrivateMember")
private val ignoreOverridden: Boolean by config(true)
Copy link
Member

Choose a reason for hiding this comment

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

This is a "breaking change". It is not in the "normal way" but we treat the yaml configuration as part of our public API.

Instead of removing the property, just keep it and add it this annotation @Deprecated("This configuration is ignored") (I'm not inspired with my descriptions feel free to improve it).

Other option is to add the deprecation atio.gitlab.arturbosch.detekt.generator.printer.writeMigratedRules. This way we keep the code clean.

What bothe options does is to keep detekt-core/src/main/resources/deprecation.properties up to date with old the deprecations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that's a clever way to manage deprecation. I have assumed that between the two ways of doing things you have stated, the first way is cleaner because the code is kept in sync with the deprecation.properties.

I just need to rebase this on the latest main before we review again. I will ping later today when it is ready 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @BraisGabin I think this is ready for you to have another look at now


override val issue = Issue(
Expand Down Expand Up @@ -67,7 +69,7 @@ class BooleanPropertyNaming(config: Config = Config.empty) : Rule(config) {
typeName == KOTLIN_BOOLEAN_TYPE_NAME || typeName == JAVA_BOOLEAN_TYPE_NAME
val isNonConstantBooleanType = isBooleanType && !declaration.isConstant()

if (isNonConstantBooleanType && !name.contains(allowedPattern) && !isIgnoreOverridden(declaration)) {
if (isNonConstantBooleanType && !name.contains(allowedPattern) && !declaration.isOverride()) {
report(reportCodeSmell(declaration, name))
}
}
Expand All @@ -91,8 +93,6 @@ 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
6 changes: 3 additions & 3 deletions ...ng/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ConstructorParameterNaming.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ class ConstructorParameterNaming(config: Config = Config.empty) : Rule(config) {
private val excludeClassPattern: Regex by config("$^") { it.toRegex() }

@Configuration("ignores constructor properties that have the override modifier")
@Deprecated("This configuration is ignored and will be removed in the future")
@Suppress("UnusedPrivateMember")
private val ignoreOverridden: Boolean by config(true)

override fun visitParameter(parameter: KtParameter) {
if (!parameter.isConstructor() ||
parameter.isContainingExcludedClassOrObject(excludeClassPattern) ||
isIgnoreOverridden(parameter)
parameter.isOverride()
) {
return
}
Expand Down Expand Up @@ -78,8 +80,6 @@ class ConstructorParameterNaming(config: Config = Config.empty) : Rule(config) {
}
}

private fun isIgnoreOverridden(parameter: KtParameter) = ignoreOverridden && parameter.isOverride()

private fun KtParameter.isConstructor(): Boolean {
return this.ownerFunction is KtConstructor<*>
}
Expand Down
5 changes: 3 additions & 2 deletions ...t-rules-naming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/FunctionNaming.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,14 @@ class FunctionNaming(config: Config = Config.empty) : Rule(config) {
private val excludeClassPattern: Regex by config("$^", String::toRegex)

@Configuration("ignores functions that have the override modifier")
@Deprecated("This configuration is ignored and will be removed in the future")
@Suppress("UnusedPrivateMember")
private val ignoreOverridden: Boolean by config(true)

override fun visitNamedFunction(function: KtNamedFunction) {
super.visitNamedFunction(function)

if (ignoreOverridden && function.isOverride()) {
if (function.isOverride()) {
return
}

Expand All @@ -65,6 +67,5 @@ class FunctionNaming(config: Config = Config.empty) : Rule(config) {
companion object {
const val FUNCTION_PATTERN = "functionPattern"
const val EXCLUDE_CLASS_PATTERN = "excludeClassPattern"
const val IGNORE_OVERRIDDEN = "ignoreOverridden"
}
}
7 changes: 4 additions & 3 deletions ...aming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/FunctionParameterNaming.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,18 @@ class FunctionParameterNaming(config: Config = Config.empty) : Rule(config) {
@Deprecated("Use `ignoreOverridden` instead")
private val ignoreOverriddenFunctions: Boolean by config(true)

@Suppress("DEPRECATION")
@OptIn(UnstableApi::class)
@Configuration("ignores overridden functions with parameters not matching the pattern")
@Deprecated("This configuration is ignored and will be removed in the future")
@Suppress("DEPRECATION", "UnusedPrivateMember")
@OptIn(UnstableApi::class)
private val ignoreOverridden: Boolean by configWithFallback(::ignoreOverriddenFunctions, true)

override fun visitParameter(parameter: KtParameter) {
if (parameter.isParameterInFunction()) {
return
}

if (ignoreOverridden && parameter.ownerFunction?.isOverride() == true) {
if (parameter.ownerFunction?.isOverride() == true) {
return
}

Expand Down
4 changes: 3 additions & 1 deletion ...t-rules-naming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/VariableNaming.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ class VariableNaming(config: Config = Config.empty) : Rule(config) {
private val excludeClassPattern: Regex by config("$^", String::toRegex)

@Configuration("ignores member properties that have the override modifier")
@Deprecated("This configuration is ignored and will be removed in the future")
@Suppress("UnusedPrivateMember")
private val ignoreOverridden: Boolean by config(true)

override fun visitProperty(property: KtProperty) {
Expand All @@ -52,7 +54,7 @@ class VariableNaming(config: Config = Config.empty) : Rule(config) {
return
}

if (ignoreOverridden && property.isOverride()) {
if (property.isOverride()) {
return
}

Expand Down