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.

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

Remove custom assertions that check kdoc of rules #3859

Merged
merged 4 commits into from
Jun 19, 2021

Conversation

marschwar
Copy link
Contributor

This is part of the #3670 cleanup.

  • removes checks that are no longer necessary after switching to annotations
  • add missing rule sets to checks
  • add dependency to formatting rules to be able to check those
  • test rule configs more generically

Comment on lines -57 to -61
for (ymlOption in filter) {
val configField = configFields.singleOrNull { ymlOption.key == it.get(null) }
if (configField == null) {
fail<String>("${ymlOption.key} option for ${ruleClass.simpleName} rule is not correctly defined")
}
Copy link
Member

Choose a reason for hiding this comment

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

This piece code is providing certain coverage for at least the rule config key "active".

Copy link
Member

Choose a reason for hiding this comment

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

Agree

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 disagree. This code explicitly ignores the active key as it is part of allowedOptions (see below).

val configFields = ruleClass.declaredFields.filter { isPublicStaticFinal(it) && it.name != "Companion" }
var filter = ymlOptions.filterKeys { it !in allowedOptions }
// ...
for (ymlOption in filter) {
  // ...
}

What this code acutally does is make sure there is a constant with the name of every config key (except for "active", "excludes", ...) such as.

const val THRESHOLD = "threshold"
const val FUNCTION_THRESHOLD = "functionThreshold"
const val CONSTRUCTOR_THRESHOLD = "constructorThreshold"
const val IGNORE_DEFAULT_PARAMETERS = "ignoreDefaultParameters"
const val IGNORE_DATA_CLASSES = "ignoreDataClasses"
const val IGNORE_ANNOTATED = "ignoreAnnotated"

for

  LongParameterList:
    active: true
    functionThreshold: 6
    constructorThreshold: 7
    ignoreDefaultParameters: false
    ignoreDataClasses: true
    ignoreAnnotated: []

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pointing out the logic, I did run the test in IDE again for the existing code, it looks like most of the rules are hitting the return branch of if (ruleClass.isConfiguredWithAnnotations()) return, and what I have observed is that when hitting for (ymlOption in filter), the filter is almost always an empty list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, as all rules have been moved to the configuration by annotation that was pretty much what I expected. Is there any check you would like me to add here?

In a separate PR I was thinking about adding something to the ConfigurationCollector that fails when there is a @configuration kdoc on a rule. Technically this could be done here as well.

Copy link
Member

Choose a reason for hiding this comment

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

Let's do it in a separate PR. Since the tests are not covering much as of today.

@cortinico cortinico added this to the 1.18.0 milestone Jun 7, 2021
@chao2zhang chao2zhang merged commit 537d6b8 into detekt:main Jun 19, 2021
@cortinico cortinico added the housekeeping Marker for housekeeping tasks and refactorings label Jul 8, 2021
@marschwar marschwar deleted the remove-config-assertions branch July 27, 2021 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants