LongMethod: add 'ignoreAnnotated' configuration option #3892
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3892 +/- ##
============================================
+ Coverage 83.51% 83.54% +0.02%
- Complexity 3118 3119 +1
============================================
Files 456 456
Lines 8992 8974 -18
Branches 1752 1746 -6
============================================
- Hits 7510 7497 -13
+ Misses 566 565 -1
+ Partials 916 912 -4
Continue to review full report at Codecov.
|
| @@ -57,6 +61,8 @@ class LongMethod(config: Config = Config.empty) : Rule(config) { | |||
| } | |||
| for ((function, lines) in functionToLines) { | |||
| if (lines >= threshold) { | |||
| val annotationExcluder = AnnotationExcluder(function.containingKtFile, ignoreAnnotated) | |||
| if (annotationExcluder.shouldExclude(function.annotationEntries)) continue | |||
There was a problem hiding this comment.
Just a thought: How about checking if the function is annotated with an ignored function in visitNamedFunction and not count the lines and put the function into the map in the first place?
| @@ -38,6 +39,9 @@ class LongMethod(config: Config = Config.empty) : Rule(config) { | |||
| @Configuration("number of lines in a method to trigger the rule") | |||
| private val threshold: Int by config(defaultValue = 60) | |||
|
|
|||
| @Configuration("ignore long methods in the context of these annotation class names") | |||
| private val ignoreAnnotated: List<String> by config(listOf("Composable")) | |||
There was a problem hiding this comment.
I don't like the idea that the detekt defaults become more and more android related. In my opinion it should focus on the kotlin language and not on frameworks like Spring or Android.
There was a problem hiding this comment.
I've implemented it the same as FunctionNaming.
There was a problem hiding this comment.
I know, your PR is totally fine. Maybe I should have started a discussion rather than raising this here. Sorry about that.
There was a problem hiding this comment.
I agree that we shouldn’t have such opinionated defaults. Sometimes it slipped through the reviews. In general, we handled it in the past like @marschwar said.
There was a problem hiding this comment.
I think we should remove this default value from both rules.
| @@ -71,6 +75,9 @@ class LongMethod(config: Config = Config.empty) : Rule(config) { | |||
| } | |||
|
|
|||
| override fun visitNamedFunction(function: KtNamedFunction) { | |||
| val annotationExcluder = AnnotationExcluder(function.containingKtFile, ignoreAnnotated) | |||
There was a problem hiding this comment.
I don't know how nit picky this is but creating the Excluder for every function seems wasteful. I prefer the way this is implemented in LongParameterList.
private lateinit var annotationExcluder: AnnotationExcluder
override fun visitKtFile(file: KtFile) {
annotationExcluder = AnnotationExcluder(file, ignoreAnnotated)
super.visitKtFile(file)
}
|
Thank you for adding this! 👏 |
Fixes #3887