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
Implement rule to check the correct usage of @RequiresTypeResolution
#5182
Conversation
It seems like that |
...es-ruleauthors/src/main/kotlin/io/gitlab/arturbosch/detekt/authors/RequiresTypeResolution.kt
Outdated
Show resolved
Hide resolved
...uleauthors/src/test/kotlin/io/gitlab/arturbosch/detekt/authors/RequiresTypeResolutionSpec.kt
Outdated
Show resolved
Hide resolved
...uleauthors/src/test/kotlin/io/gitlab/arturbosch/detekt/authors/RequiresTypeResolutionSpec.kt
Outdated
Show resolved
Hide resolved
I thought the same. But they compile successfully. |
f6a5567
to
6b50290
Compare
0f8cb03
to
f6692e1
Compare
1f289af
to
c720dc1
Compare
c720dc1
to
d024ed0
Compare
Codecov Report
@@ Coverage Diff @@
## main #5182 +/- ##
===========================================
+ Coverage 0 85.97% +85.97%
- Complexity 0 3614 +3614
===========================================
Files 0 514 +514
Lines 0 12056 +12056
Branches 0 2162 +2162
===========================================
+ Hits 0 10365 +10365
- Misses 0 618 +618
- Partials 0 1073 +1073
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
f6692e1
to
6d37691
Compare
2830585
to
6c2c21a
Compare
25f122d
to
239d04d
Compare
This should be ready to review :) |
|
||
override fun visitKtFile(file: KtFile) { | ||
super.visitKtFile(file) | ||
klasses.forEach { klass -> |
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.
is it possible to move the checking logic inside visitClass(klass: KtClass)
to avoid maintaining klass: MutableList<KtClass>
?
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.
The problem is that we have rules that uses the BindingContext
inside private extension functions. So if I move it to visitClass
we would hava false positives. For that reason I moved it to visitClass
. I don't like the MutableList
either but i don't know other way to implement it.
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'm wondering if the check here is too sophisticated and we should just do a syntactic check on whether BindingContext
or bindingContext
are present as string in the body of the rule.
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.
That was my initial implementation but I got this comment: #5182 (comment) so I made it more sophisticated. I think it's better like it's now.
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.
Let's make sure we ship this inside RC2 otherwise we'll end up shipping an empty ruleset :)
import kotlin.reflect.KClass | ||
|
||
/** | ||
* If a rule uses `bindingContext` should be annotated with `@RequiresTypeResolution`. And if it doesn't it shouldn't |
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.
* If a rule uses `bindingContext` should be annotated with `@RequiresTypeResolution`. And if it doesn't it shouldn't | |
* If a rule uses [BindingContext] should be annotated with `@RequiresTypeResolution`. And if it doesn't it shouldn't |
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.
Here I'm talking about the bindingContext
property. No the type BindingContext
. I rephrased it to be more clear.
...es-ruleauthors/src/main/kotlin/io/gitlab/arturbosch/detekt/authors/RequiresTypeResolution.kt
Outdated
Show resolved
Hide resolved
|
||
override fun visitKtFile(file: KtFile) { | ||
super.visitKtFile(file) | ||
klasses.forEach { klass -> |
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'm wondering if the check here is too sophisticated and we should just do a syntactic check on whether BindingContext
or bindingContext
are present as string in the body of the rule.
...es-ruleauthors/src/main/kotlin/io/gitlab/arturbosch/detekt/authors/RequiresTypeResolution.kt
Outdated
Show resolved
Hide resolved
@cortinico can you re-review this? I adressed some of the issues you pointed out and comment in other that I'm not 100% sure if they should be implemented. |
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.
Minor typo in the rule name. Other than that, looks good to me 👍
config/detekt/detekt.yml
Outdated
@@ -271,3 +271,8 @@ style: | |||
WildcardImport: | |||
active: true | |||
excludeImports: [] | |||
|
|||
ruleauthors: | |||
ViolateTypeResolutionRequirements: |
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.
ViolateTypeResolutionRequirements: | |
ViolatesTypeResolutionRequirements: |
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.
We also have UseX
as imperative sentences though, so I don't think it is a typo.
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.
But we also use Has
(HasPlatformType
) which is a plural.
To me the UseX
rules (like UseRequire
) should be interpreted in a sentence like "You should use require here".
While the one with plurarls should be interpreted in sentences where the subject is "the code" so like "your code has platform type" or "your code violates type resolution".
Also this is a nit, so feel free to skip it :)
...ors/src/main/kotlin/io/gitlab/arturbosch/detekt/authors/ViolateTypeResolutionRequirements.kt
Outdated
Show resolved
Hide resolved
@BraisGabin can we finalize this as I'd like to prepare RC2 tomorrow 👍 |
I don't have a computer near this weekend. I'm OK with keeping the current name or adding the If you want to release tomorrow feel free to change the name yourself. |
…detekt/authors/RequiresTypeResolutionSpec.kt Co-authored-by: marschwar <marschwar@users.noreply.github.com>
bb84d73
to
c4bdd5c
Compare
Fixes #5148
This rule detects all the mixed behaviour rules that we have on our project so I added them to the baseline meanwhile we don't fix them. But at least with this rule we ensure that we don't add any new mixed and we know all the current mixed rules too.
I know that the test are failing but I have no idea why the tests are failing. I debugged andOnly returns aList
with a single element:kotlin.Any
. But when I run the rule over the code base of detekt it seems to work. Any idea?