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

Improve LongParameterList rule by supporting ignoring annotated parameters #3879

Merged
merged 3 commits into from
Jun 19, 2021

Conversation

andrepaz
Copy link
Contributor

Add support for ignoring annotated parameters in constructors and functions in addition to the other forms of ignoring already supported by the LongParameterList rule.

It should be useful for annotations such as Spring's @value that mark something that usually doesn't actually add a dependency, just a custom form of configuration (similar to what a parameter with a default value would do) for a function or class and as such might not violate the Single Responsibility Principle as stated by the rule's own documentation of its motivation.

Users should be able to decide if annotated parameters should count or not, just as they can choose or not to ignore parameters with a default value.

Add support for ignoring annotated parameters in constructors and
functions in addition to the other forms of ignoring already supported.
It should be useful for annotations such as Spring's @value.
@codecov
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #3879 (ad427c9) into main (158744f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #3879   +/-   ##
=========================================
  Coverage     83.50%   83.50%           
- Complexity     3115     3117    +2     
=========================================
  Files           456      456           
  Lines          9001     9002    +1     
  Branches       1757     1758    +1     
=========================================
+ Hits           7516     7517    +1     
  Misses          566      566           
  Partials        919      919           
Impacted Files Coverage Δ
...bosch/detekt/rules/complexity/LongParameterList.kt 89.79% <100.00%> (+0.21%) ⬆️

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 158744f...ad427c9. Read the comment docs.

Though isolated test execution of the detekt-rules-complexity module
worked in previous version of LongParameterList improvement, another
step of the building process less lenient than the previous one failed
because the annotations that were used were not properly configured to
target value parameters. A new annotation had to be added to the test
configuration to work around that issue.

Additionally, another test case scenario was added to improve test
coverage.
Copy link

@rmzoni rmzoni left a comment

Choose a reason for hiding this comment

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

Great work @andrepaz

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.

Awesome contribution! Thank you. Could you please add this to the documentation in the rule class in order to make this change transparent for users?

@andrepaz
Copy link
Contributor Author

Awesome contribution! Thank you. Could you please add this to the documentation in the rule class in order to make this change transparent for users?

Sure. I've changed the text to mention explicit support for parameters as well as the other annotation forms. Is that OK?

@cortinico cortinico added this to the 1.18.0 milestone Jun 19, 2021
Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Great work 👍

@cortinico cortinico merged commit 85533a0 into detekt:main Jun 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants