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
Fix AnnotationExcluder
#4518
Fix AnnotationExcluder
#4518
Conversation
0900eea
to
376ac23
Compare
detekt-psi-utils/src/main/kotlin/io/github/detekt/psi/FullQualifiedNameGuesser.kt
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #4518 +/- ##
============================================
+ Coverage 84.11% 84.41% +0.30%
- Complexity 3312 3323 +11
============================================
Files 477 479 +2
Lines 10920 11129 +209
Branches 2029 2037 +8
============================================
+ Hits 9185 9395 +210
Misses 700 700
+ Partials 1035 1034 -1
Continue to review full report at Codecov.
|
376ac23
to
b549bb1
Compare
] | ||
) | ||
fun `all cases`(exclusion: String, annotation: String, shouldExclude: Boolean) { | ||
val excluder = AnnotationExcluder(file, listOf(exclusion)) |
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.
Non blocking comment: I hate parameterized tests like this. That is why I split it into 2 tests. But I know there are more schools of thought here ;) But that is something for our style guide. How many parameters may a test have before it is considered unreadable.
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 don't like this kind of tests either. I would prefer to have way less. But they were needed. There were really random cases that were not covered. And while I was implementing this I broke some of them while fixing others.
And I think that it's better to have all the cases together because they are in blocks of 5. Maybe I could add a new line between each "block of tests" to make it easier to read. But I can reverse the change and split them in two if you think it's better.
} | ||
} | ||
|
||
private val kotlinPackageClasses = arrayOf( |
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.
A couple of things here:
- I would move it in a separate file.
- Is it copied from somewhere in the Kotlin repo? If so, please provide a reference so we can keep it up to date easily.
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.
Both of them done. Thanks to force the point 2. I was missing lots of classes. I couldn't find a list with this information so I created a script to extract it from the documentation of detekt.
Basically I download the documentation and parse the html. And I extracted the list of packages to parse from the documentation (all explained in the comment)
b549bb1
to
b4c9bd6
Compare
b4c9bd6
to
7d35ecf
Compare
#4368 showed that
AnnotationExcluder
had some issues: false-positives and false-negatives.This PR implements a
FullQualifiedNameGuesser
to archive that. It's a guesser so there are some scenarios where we can't know for sure which class we are refering too. But in general it only returns one option. This is a best-effort for the users that doesn't use type-solving. With type-solving we have other implementation that works all the times.