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

Add ignoreOverridden option for TooManyFunctions rule - #1411 #1488

Merged
merged 1 commit into from
Feb 17, 2019
Merged

Add ignoreOverridden option for TooManyFunctions rule - #1411 #1488

merged 1 commit into from
Feb 17, 2019

Conversation

IvanAntsiferov
Copy link
Contributor

closes #1411

@@ -117,6 +117,7 @@ complexity:
thresholdInEnums: 11
ignoreDeprecated: false
ignorePrivate: false
ignoreOverridden: false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be true by default.

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 the contribution. Please remove this config option and just ignore enums by default. Otherwise, the code looks fine. Thanks for your contribution! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@schalkms Did you mean ignore overriden functions by default without additional rule option?

Copy link
Member

@arturbosch arturbosch Feb 17, 2019

Choose a reason for hiding this comment

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

I also don't understand what @schalkms wrote :)? Active by default is okay for me too ^^

Copy link
Member

Choose a reason for hiding this comment

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

I am really sorry. I mixed it up with the proposal to ignore enum classes in #1489
Everything is fine.

Copy link
Member

Choose a reason for hiding this comment

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

Well maybe it shouldn't be true by default. Better to be strict by default and relaxed when needed.
It also aligns with the other ignore options.

@codecov-io
Copy link

codecov-io commented Feb 16, 2019

Codecov Report

Merging #1488 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1488      +/-   ##
============================================
+ Coverage     74.74%   74.75%   +<.01%     
- Complexity     1732     1734       +2     
============================================
  Files           328      328              
  Lines          5270     5272       +2     
  Branches        957      958       +1     
============================================
+ Hits           3939     3941       +2     
  Misses          805      805              
  Partials        526      526
Impacted Files Coverage Δ Complexity Δ
...rbosch/detekt/rules/complexity/TooManyFunctions.kt 94.82% <50%> (+0.18%) 24 <0> (+2) ⬆️

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 7025536...2b33c92. Read the comment docs.

@@ -124,6 +127,7 @@ class TooManyFunctions(config: Config = Config.empty) : Rule(config) {
private fun isIgnoredFunction(function: KtNamedFunction): Boolean = when {
ignoreDeprecated && function.annotationEntries.any { it.typeReference?.text == DEPRECATED } -> true
ignorePrivate && function.isPrivate() -> true
ignoreOverridden && (function.modifierList?.hasModifier(KtTokens.OVERRIDE_KEYWORD) ?: false) -> true
Copy link
Member

Choose a reason for hiding this comment

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

Please use function.isOverride() here.

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 did not notice that the project already has an extension function for that.

Copy link
Member

Choose a reason for hiding this comment

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

No problem, thanks for contributing :)!

@arturbosch arturbosch added this to the Upcoming milestone Feb 17, 2019
@arturbosch arturbosch merged commit f5a0f28 into detekt:master Feb 17, 2019
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.

TooManyFunctions flags overridden functions
5 participants