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

Fix AnnotationExcluder #4518

Merged
merged 6 commits into from Jan 31, 2022
Merged

Fix AnnotationExcluder #4518

merged 6 commits into from Jan 31, 2022

Conversation

BraisGabin
Copy link
Member

@BraisGabin BraisGabin commented Jan 24, 2022

#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.

@codecov
Copy link

codecov bot commented Jan 24, 2022

Codecov Report

Merging #4518 (fab3a88) into main (5176e41) will increase coverage by 0.30%.
The diff coverage is 98.18%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
...ub/detekt/psi/internal/FullQualifiedNameGuesser.kt 89.28% <89.28%> (ø)
...gitlab/arturbosch/detekt/api/AnnotationExcluder.kt 94.73% <93.75%> (+13.48%) ⬆️
...ithub/detekt/psi/internal/KotlinNoImportClasses.kt 100.00% <100.00%> (ø)
...in/io/gitlab/arturbosch/detekt/rules/style/Junk.kt 75.00% <0.00%> (-25.00%) ⬇️
...n/io/github/detekt/report/html/HtmlOutputReport.kt 95.50% <0.00%> (-0.05%) ⬇️
...otlin/io/gitlab/arturbosch/detekt/core/TaskPool.kt 100.00% <0.00%> (ø)
...rturbosch/detekt/rules/style/TrailingWhitespace.kt 96.29% <0.00%> (+0.29%) ⬆️
...lab/arturbosch/detekt/rules/style/MaxLineLength.kt 95.00% <0.00%> (+0.40%) ⬆️
...in/kotlin/io/github/detekt/report/xml/XmlEscape.kt 65.97% <0.00%> (+1.58%) ⬆️

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 5176e41...fab3a88. Read the comment docs.

]
)
fun `all cases`(exclusion: String, annotation: String, shouldExclude: Boolean) {
val excluder = AnnotationExcluder(file, listOf(exclusion))
Copy link
Contributor

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.

Copy link
Member Author

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(
Copy link
Member

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:

  1. I would move it in a separate file.
  2. Is it copied from somewhere in the Kotlin repo? If so, please provide a reference so we can keep it up to date easily.

Copy link
Member Author

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)

@BraisGabin BraisGabin merged commit 2d00cab into main Jan 31, 2022
@BraisGabin BraisGabin deleted the FullQualifiedNameGuesser branch January 31, 2022 09:19
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.

None yet

4 participants