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

Use annotations to configure rules in rules-complexity #3768

Merged
merged 14 commits into from
May 22, 2021

Conversation

marschwar
Copy link
Contributor

This belongs to #3670 and replaces all configuration kdoc tags in rules-complexity with annotations. There are a few decisions to make:

  • Named arguments vs. positional arguments
  • Method references vs. lambda

simple properties

private val prop: Boolean by config(defaultValue = false)
// or
private val prop: Boolean by config(false)

fallback properties

private val prop: Int by configWithFallback(fallbackPropertyName = "threshold", defaultValue = 1)
// or 
private val prop: Int by configWithFallback("threshold", 1)

transformed properties

private val prop: Regex by config("$^", String::toRegex)
// or
private val prop: Regex by config("$^") { it.toRegex() }

I am fairly ambivalent about this and I don't know if that is something we even have to agree upon. I am curious about your thoughts.

@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #3768 (e866330) into main (21b7d7c) will decrease coverage by 0.06%.
The diff coverage is 98.07%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3768      +/-   ##
============================================
- Coverage     83.52%   83.46%   -0.07%     
+ Complexity     2915     2900      -15     
============================================
  Files           452      452              
  Lines          8765     8749      -16     
  Branches       1665     1665              
============================================
- Hits           7321     7302      -19     
- Misses          542      545       +3     
  Partials        902      902              
Impacted Files Coverage Δ Complexity Δ
...bosch/detekt/rules/complexity/LabeledExpression.kt 87.50% <66.66%> (ø) 14.00 <0.00> (ø)
...rbosch/detekt/rules/complexity/ComplexCondition.kt 87.80% <100.00%> (ø) 10.00 <1.00> (ø)
...rbosch/detekt/rules/complexity/ComplexInterface.kt 90.62% <100.00%> (-3.13%) 13.00 <0.00> (-1.00)
...rturbosch/detekt/rules/complexity/ComplexMethod.kt 95.23% <100.00%> (ø) 13.00 <1.00> (ø)
...b/arturbosch/detekt/rules/complexity/LargeClass.kt 96.96% <100.00%> (-0.18%) 10.00 <0.00> (-2.00)
...b/arturbosch/detekt/rules/complexity/LongMethod.kt 88.37% <100.00%> (-0.52%) 15.00 <1.00> (-1.00)
...bosch/detekt/rules/complexity/LongParameterList.kt 89.36% <100.00%> (+0.72%) 22.00 <1.00> (-1.00) ⬆️
...bosch/detekt/rules/complexity/MethodOverloading.kt 91.66% <100.00%> (-0.44%) 3.00 <0.00> (-2.00)
...turbosch/detekt/rules/complexity/NamedArguments.kt 83.33% <100.00%> (-1.67%) 13.00 <0.00> (-2.00)
...rbosch/detekt/rules/complexity/NestedBlockDepth.kt 91.66% <100.00%> (-0.34%) 3.00 <0.00> (-2.00)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21b7d7c...e866330. Read the comment docs.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this initiative forward. This really improves detekt's code base. No misuse of the comments anymore.

I have a few open questions.

  • I guess the generator module also needs an update in order to output the correct website sources. Am I right?
  • Do we need any other updates to the documentation? I'm thinking about the Contributing.md in order to not mislead new contributors or users. Detekt's docs become outdated quite often. It's sometimes forgotten when introducing changes.

@marschwar
Copy link
Contributor Author

The generator module and the contribution guide have already been updated.

@schalkms
Copy link
Member

The generator module and the contribution guide have already been updated.

Cool. I missed this, unfortunately.

@cortinico cortinico added this to the 1.18.0 milestone May 14, 2021
@@ -110,6 +112,6 @@ class ComplexCondition(
}

companion object {
const val DEFAULT_CONDITIONS_COUNT = 4
private const val DEFAULT_CONDITIONS_COUNT = 4
Copy link
Member

Choose a reason for hiding this comment

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

Probably out of scope but I think that inline this values will improve the readability of this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point. I was afraid to trigger magic number violations but it seems to work


override val issue = Issue(
"NamedArguments",
Severity.Maintainability,
"Function invocation with more than $threshold parameters must all be named",
"Parameters of function invocation must all be named",
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use the threshold? The description that you set here is not correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a great catch. Unfortunately that is not possible. When threshold is evaluated, the ruleId is used to lookup the config key. In order to get the ruleId, the issue is created. -> infinite loop

How about: "Prefer named arguments for functions to improve readability." This is semi-consistent with rules like ComplexInterface or ComplexCondition where only the general rule is described without mentioning the exact threshold.

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 actually a bug today: Even if the threshold in configuration is 4, the issue.description will always be Function invocation with more than 3 parameters must all be named because the existing description is reading value from the threshold constructor argument.

@BraisGabin BraisGabin merged commit 9494780 into detekt:main May 22, 2021
@marschwar marschwar deleted the annotation-complexity branch May 23, 2021 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants