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

Adds ForbiddenSuppress rule #4899

Merged
merged 1 commit into from Jun 3, 2022

Conversation

gfreivasc
Copy link
Contributor

This rule is based on discussion #4890. It has been added to the "style"
rule set, since I believe it is opinionated and usage would vary across code
bases.

Checks for suppression of one or more rules that have been decided to
never be suppressed within a code base. For example, if we've set
MaximumLineLength to a value, every file must be compliant, and if there
is a special case where compliance is discouraged, we can simply exclude
this file within YML configuration.

This rule cannot prevent itself from being suppressed, but should serve
to discourage the over use of @Suppress annotations.

@codecov
Copy link

codecov bot commented Jun 2, 2022

Codecov Report

Merging #4899 (d0c9ffc) into main (4cc4051) will increase coverage by 0.02%.
The diff coverage is 85.29%.

@@             Coverage Diff              @@
##               main    #4899      +/-   ##
============================================
+ Coverage     84.80%   84.82%   +0.02%     
- Complexity     3488     3511      +23     
============================================
  Files           495      497       +2     
  Lines         11464    11549      +85     
  Branches       2122     2139      +17     
============================================
+ Hits           9722     9797      +75     
- Misses          685      687       +2     
- Partials       1057     1065       +8     
Impacted Files Coverage Δ
...arturbosch/detekt/rules/style/ForbiddenSuppress.kt 84.84% <84.84%> (ø)
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <100.00%> (ø)
...osch/detekt/rules/complexity/ComplexityProvider.kt 100.00% <0.00%> (ø)
...ch/detekt/rules/complexity/NestedScopeFunctions.kt 90.00% <0.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 4cc4051...d0c9ffc. Read the comment docs.

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 stuff 👍 Thanks for taking the time to send this over
:)

override val issue = Issue(
javaClass.simpleName,
Severity.Maintainability,
"",
Copy link
Member

Choose a reason for hiding this comment

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

This should not be left empty 👍

.find { it is KtStringTemplateExpression }
?.children
?.find { it is KtLiteralStringTemplateEntry }
val text = stringTemplate?.text
Copy link
Member

Choose a reason for hiding this comment

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

This can be chained with the rest?

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 extracted that because I was following along debugging the code, will add to the chain.

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.

I second Nicos comments.
Thanks for this awesome contribution! 👍

Comment on lines 19 to 21
* This rule allows to set a list of rules whose suppression is forbidden. This can be used to discourage abuse of the
* `Suppress` and `SuppressWarnings` annotations. Detekt will report suppression of all forbidden rules. This rule is
* not capable of reporting suppression of itself, as that's a language feature with precedence.
Copy link
Member

Choose a reason for hiding this comment

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

minor formatting and grammar

Suggested change
* This rule allows to set a list of rules whose suppression is forbidden. This can be used to discourage abuse of the
* `Suppress` and `SuppressWarnings` annotations. Detekt will report suppression of all forbidden rules. This rule is
* not capable of reporting suppression of itself, as that's a language feature with precedence.
* This rule allows to set a list of rules whose suppression is forbidden.
* This can be used to discourage the abuse of the `Suppress` and `SuppressWarnings` annotations.
* Detekt will report the suppression of all forbidden rules.
* This rule is not capable of reporting the suppression of itself, as that's a language feature with precedence.

@gfreivasc gfreivasc force-pushed the gabrielfv/forbidden-suppress branch from 7aac082 to e439cc7 Compare June 2, 2022 18:37
This rule is based on discussion detekt#4890.

Checks for suppression of one or more rules that have been decided to
never be suppressed within a code base. For example, if we've set
MaximumLineLength to a value, every file must be compliant, and if there
is a special case where compliance is discouraged, we can simply exclude
this file within YML configuration.

This rule cannot prevent itself from being suppressed, but should serve
to discourage the over use of `@Suppress` annotations.
@gfreivasc
Copy link
Contributor Author

@cortinico @schalkms addressed your comments.

@cortinico cortinico added this to the 1.21.0 milestone Jun 3, 2022
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.

LGTM 👍 Thank you for sending this

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.

None yet

3 participants