Rule Configuration using annotations by marschwar · Pull Request #3637 · detekt/detekt

marschwar · 2021-03-30T19:19:52Z

This PR relates to #3562 and makes it possible to configure a Rule using a @Configuration annotation on properties in conjunction with a config delegate.

Advantages

The default value for a config parameter is detected automatically
The name for a config parameter is automatically the same as the property
There is no need to call valueOrDefault as this is done by the delegate automatically

Limitations / Points to discuss

Right now this is only implemented on one rule ComplexMethod to illustrate what it would look like.
Currently only String, Boolean, Int, Long and List are supported as configuration properties. Which types did I miss?
The default value can only be inferred when it is definied directly in the delegate expression or as a constant in the rules companion object. (see Type resolution in generator module #3628)
The formatting of lists of strings in the default-detekt-config.yml is inconsistent ([run, let, apply] vs. ['**/test/**', '**/androidTest/**']) so I opted for the version with quotes. Is that a problem?
I disabled the checks in ConfigAssert for rules that use @Configuration as they are not needed IMO.

BraisGabin · 2021-03-30T19:56:40Z

This PR relates to #3562 and makes it possible to configure a Rule using a @Configuration annotation on properties in conjunction with a config delegate.

Advantages
* The default value for a config parameter is detected automatically

* The name for a config parameter is automatically the same as the property

* There is no need to call `valueOrDefault` as this is done by the delegate automatically
Limitations / Points to discuss

Currently only String, Boolean, Int, Long and List are supported as configuration properties. Which types did I miss?

Regex? It's the same as String but maybe we could use it to make more clear what type of String we expect there. But this is out of scope of this PR. It could be done as a follow up.

The default value can only be inferred when it is definied directly in the delegate expression or as a constant in the rules companion object. (see Type resolution in generator module #3628)

I don't know how to help you here. Are there a lot of these cases? Could we simplify them to use always values in the same file?

The formatting of lists of strings in the default-detekt-config.yml is inconsistent ([run, let, apply] vs. ['**/test/**', '**/androidTest/**']) so I opted for the version with quotes. Is that a problem?

This is not a problem at all. I was wondering about other configurations where we use this syntax:

    exceptionNames:
     - ArrayIndexOutOfBoundsException
     - Error
     - Exception
     - IllegalMonitorStateException
     - NullPointerException
     - IndexOutOfBoundsException
     - RuntimeException
     - Throwable

Could it be possible to use both? I mean, the one-line syntax in this example would be really difficult to maintain and works really bad with git.

But again this don't need to be faced in this PR if it makes it more complex.

codecov · 2021-03-30T20:06:43Z

Codecov Report

Merging #3637 (d97e8d7) into main (4495b36) will decrease coverage by 0.01%.
The diff coverage is 77.50%.

❗ Current head d97e8d7 differs from pull request most recent head f91f3a6. Consider uploading reports for the commit f91f3a6 to get more accurate results

@@             Coverage Diff              @@
##               main    #3637      +/-   ##
============================================
- Coverage     77.97%   77.95%   -0.02%     
- Complexity     2837     2870      +33     
============================================
  Files           467      470       +3     
  Lines          9139     9245     +106     
  Branches       1737     1761      +24     
============================================
+ Hits           7126     7207      +81     
- Misses         1071     1077       +6     
- Partials        942      961      +19

Impacted Files	Coverage Δ	Complexity Δ
...t/generator/collection/RuleSetProviderCollector.kt	`75.47% <0.00%> (-1.89%)`	`4.00 <0.00> (ø)`
...ekt/generator/collection/DocumentationCollector.kt	`70.00% <70.00%> (ø)`	`15.00 <15.00> (?)`
...ekt/generator/collection/ConfigurationCollector.kt	`70.58% <70.58%> (ø)`	`20.00 <20.00> (?)`
...b/arturbosch/detekt/api/internal/ConfigProperty.kt	`80.00% <80.00%> (ø)`	`0.00 <0.00> (?)`
...urbosch/detekt/generator/collection/Annotations.kt	`71.42% <100.00%> (+16.88%)`	`0.00 <0.00> (ø)`
...urbosch/detekt/generator/collection/RuleVisitor.kt	`89.65% <100.00%> (+6.01%)`	`33.00 <9.00> (-2.00)`	⬆️
...rturbosch/detekt/rules/complexity/ComplexMethod.kt	`95.34% <100.00%> (+1.06%)`	`13.00 <2.00> (ø)`

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 4495b36...f91f3a6. Read the comment docs.

marschwar · 2021-03-30T20:07:13Z

I don't think there are many cases where the default is defined outside the rule class itself. I don't see this as a huge problem as of know.

List format: Maybe it always should be the line-per-item syntax for all list parameters?

BraisGabin · 2021-04-04T13:13:56Z

List format: Maybe it always should be the line-per-item syntax for all list parameters?

I'm not sure about this either. We have some configurations that will probably never change ([run, let, apply]) and they looks better in just one line... But this should not be a blocker. We can fix this later.

picklebento · 2021-04-06T17:51:45Z

Could someone confirm that I have left comments before on this PR?

Instead of git push --force, what I would recommend is to git fetch + git merge when we want to add more commits to a published PR. This will preserve the conversation history while force-push may wipe out all the conversations.

marschwar · 2021-04-06T18:07:41Z

As far as I know you had not commented on this PR but rather #3608. I created an entirely new PR. I probably should have referenced it in the description.

I did force push multiple times after rebasing onto master.

picklebento

PR looks great to me!

picklebento · 2021-04-07T03:41:21Z

+class ComplexMethod(config: Config = Config.empty) : Rule(config) {
+
+    @Configuration("McCabe's Cyclomatic Complexity (MCC) number for a method.")
+    private val threshold: Int by config(DEFAULT_THRESHOLD_METHOD_COMPLEXITY)


I would personally inline 15.. I guess that's violated by the MagicNumber rule.

I am torn. I really think it is redundant since it is clearly the default value and does not need extra naming. But on the other hand I dislike us excessively suppressing our own rules.

To make this work, the annotation needs to be available at runtime.

Co-authored-by: Chao Zhang <zhangchao6865@gmail.com>

BraisGabin

And after this we need to make the migration and then remove the old code. Are you traking the remaining taask somewhere? I could help you with this more "mechanical" work.

BraisGabin · 2021-04-07T11:16:16Z

And, of course, this is a huge improvement!

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

marschwar · 2021-04-07T11:34:51Z

Are you traking the remaining taask somewhere?

What would be a good format for this? Create an issue per task or one for all?

BraisGabin · 2021-04-07T11:50:32Z

What would be a good format for this? Create an issue per task or one for all?

That's your call. Both seem fine to me.

marschwar force-pushed the annotations-configuration branch from d595335 to 5450dab Compare March 30, 2021 19:47

marschwar force-pushed the annotations-configuration branch from 71fd3d0 to 7233c4e Compare April 4, 2021 12:55

picklebento approved these changes Apr 7, 2021

View reviewed changes

Markus Schwarz and others added 18 commits April 7, 2021 12:12

Create initial idea for @configuration annotation

ef52d82

Replace map with mapNotNull

472cd68

Reorder imports

ae24d45

Use config parameter constants again

da94a93

Reorganize test

91a8fdd

Generate documentation from @configuration

ec18be0

Introduce config delegate and extract default value from it

88ed75b

Exclude rules configured with annotation from checks

f6d1f8e

To make this work, the annotation needs to be available at runtime.

Remove support for factory methods in RuleSetProviderCollector

d79adc0

Cleanup complex method spek

80f142c

Add support for lists that are empty by default

3830116

Group similar test cases

746f1f8

Rearrange code, extract documentation collector

f4bfd1d

Update documentation for rule contribution

f7629a2

Restrict config delegate to supported types

4f00d13

Use let instead of also

a55f461

Co-authored-by: Chao Zhang <zhangchao6865@gmail.com>

Replace extension property with extension function

ee4e5fc

Compare to expected configuration instance

055c2f7

marschwar force-pushed the annotations-configuration branch from 448e943 to 055c2f7 Compare April 7, 2021 10:37

BraisGabin approved these changes Apr 7, 2021

View reviewed changes

Comment thread .github/CONTRIBUTING.md Outdated

Update .github/CONTRIBUTING.md

f91f3a6

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

picklebento approved these changes Apr 8, 2021

View reviewed changes

picklebento added this to the 1.17.0 milestone Apr 8, 2021

picklebento merged commit cbee5e7 into detekt:main Apr 8, 2021

marschwar deleted the annotations-configuration branch April 9, 2021 11:16

This was referenced Apr 10, 2021

Use @Configuration annotation instead of kdoc in all rules #3670

Closed

Use dynamic list format for default-detekt-config.yml #3671

Closed

Add regex support for config delegate #3672

Closed

cortinico added the notable changes Marker for notable changes in the changelog label May 1, 2021

marschwar mentioned this pull request May 22, 2021

Configure rule sets in a safe way #3816

Closed

Uh oh!

Conversation

marschwar commented Mar 30, 2021

Advantages

Limitations / Points to discuss

Uh oh!

BraisGabin commented Mar 30, 2021

Advantages

Limitations / Points to discuss

Uh oh!

codecov Bot commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marschwar commented Mar 30, 2021

Uh oh!

BraisGabin commented Apr 4, 2021

Uh oh!

picklebento commented Apr 6, 2021

Uh oh!

marschwar commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picklebento left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picklebento Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

marschwar Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

BraisGabin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BraisGabin commented Apr 7, 2021

Uh oh!

marschwar commented Apr 7, 2021

Uh oh!

BraisGabin commented Apr 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Mar 30, 2021 •

edited

Loading

marschwar commented Apr 6, 2021 •

edited

Loading