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
Don't run rules annotated with @RequiresTypeResolution
when running plain detekt
#5176
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5176 +/- ##
============================================
+ Coverage 84.93% 85.24% +0.31%
+ Complexity 3716 3700 -16
============================================
Files 514 514
Lines 12068 12004 -64
Branches 2271 2222 -49
============================================
- Hits 10250 10233 -17
+ Misses 698 696 -2
+ Partials 1120 1075 -45
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this would be a good solution. As it was mentioned your other PR this would still require a custom rule to verify that a rule that is not annotated with @RequiresTypeResolution
does not in fact access the BindingContext
.
detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think putting the filter in core
is a good idea.
I want to get rid of the mixed rule behavior rather sooner than later.
This is for sure a breaking change and we should be careful with activating this new behavior.
To avoid confusion, I'd rather change this PR to a draft in the meantime.
7076dfc
to
6255584
Compare
a3aa554
to
69a9b90
Compare
I'm moved this to Draft and I removed all the This PR just makes the plain-detekt faster. This should be just the first step for #3577 and it is an alternative over #5130. |
I'm moving this back to "ready for review" because I don't see how this PR could be a breaking change. |
|
Not technically a breaking change, but more like a behavioural change. We should make clear that |
|
We should. As if a user wrote a rule with type resolution and haven't used the The fact that lives inside the |
It is be the other way around. If a third party rule was written and it has the annotation To me that seems a strange case, and we should be free to introduce that breaking change because no one should be using our internal api. And I agree, we should think about moving this annotation and friends to our public api. Because of this PR and the one by @VitalyVPinchuk about creating the config file for third party. But we can do that on other pr. |
Yup in this case, it makes sense.
Yup. Still worth a note in the release notes. |
detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt
Show resolved
Hide resolved
Is there something that I need to do here to move this PR forward? I think it's ready to merge but maybe I'm missunderstanding some comments that require me to change something. |
@BraisGabin no comments or requests for change from my side |
…e BindingContext is empty
…nalyzer.kt Co-authored-by: marschwar <marschwar@users.noreply.github.com>
69a9b90
to
5c1a3b9
Compare
This is an alternative for #5130. To be more precise this was proposed by @marschwar here: #5130 (comment)
I implemented this in
:detekt-core
because I think it should be the one that decides if a rule should or should not be runned. But to be more consistent with the current code we could move this check to theRule
inside:detekt-api
. What do you think?This PR isn't finish yet. I need to remove all theif (bindingContext == BindingContext.EMPTY) return
that we have in our code now but I want feedback to know if this is what we want.