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

MultiRule should pass correctly the BindingContext #4071

Merged
merged 2 commits into from
Aug 24, 2021

Conversation

cortinico
Copy link
Member

Fixes #4058

The rule BooleanPropertyNaming that was recently added relies on Type Resolution. That rule was plugged inside a MultiRule instead of a classical RuleSetProvider.

Given that rules rely on inheritance to access a BindingContext, the MultiRule should pass over the BindingContext to the rules he's managing.

The result was that BooleanPropertyNaming was effectively not running at all. This PR is fixing it.

@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #4071 (dde495d) into main (026066f) will increase coverage by 0.00%.
The diff coverage is 50.00%.

❗ Current head dde495d differs from pull request most recent head 702d905. Consider uploading reports for the commit 702d905 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main    #4071   +/-   ##
=========================================
  Coverage     83.58%   83.58%           
- Complexity     3186     3187    +1     
=========================================
  Files           459      459           
  Lines          9099     9101    +2     
  Branches       1772     1772           
=========================================
+ Hits           7605     7607    +2     
  Misses          561      561           
  Partials        933      933           
Impacted Files Coverage Δ
.../io/gitlab/arturbosch/detekt/generator/out/Yaml.kt 68.57% <0.00%> (ø)
.../arturbosch/detekt/rules/style/UseIsNullOrEmpty.kt 57.33% <50.00%> (ø)
...otlin/io/gitlab/arturbosch/detekt/api/MultiRule.kt 100.00% <100.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 026066f...702d905. Read the comment docs.

Copy link
Member

@chao2zhang chao2zhang left a comment

Choose a reason for hiding this comment

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

Good find!!!

@@ -168,7 +168,7 @@ class UseIsNullOrEmpty(config: Config = Config.empty) : Rule(config) {

private val stringClass = StandardNames.FqNames.string.toSafe()

private val isEmptyFunctions = collectionClasses.map { FqName("$it.isEmpty") } +
private val emptyCheckFunctions = collectionClasses.map { FqName("$it.isEmpty") } +
Copy link
Member

Choose a reason for hiding this comment

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

Is this a renaming that's not relevant? Just checking if we are accidentally cherry-picking other commits.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's relevant actually. BooleanPropertyNaming was not the only rule affected by this. The NonBooleanPropertyPrefixedWithIs was affected as well and now got triggered on the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

It is now catching issues. Got it!

@chao2zhang chao2zhang merged commit 3ecac0d into detekt:main Aug 24, 2021
cortinico added a commit to cortinico/detekt that referenced this pull request Aug 30, 2021
* MultiRule should pass correctly the BindingContext

* Fix violation of NonBooleanPropertyPrefixedWithIs
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.

BooleanPropertyNaming not reporting any violation
4 participants