Skip to content

Fix AnnotationExcluder#4518

Merged
BraisGabin merged 6 commits into
mainfrom
FullQualifiedNameGuesser
Jan 31, 2022
Merged

Fix AnnotationExcluder#4518
BraisGabin merged 6 commits into
mainfrom
FullQualifiedNameGuesser

Conversation

@BraisGabin
Copy link
Copy Markdown
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
Copy Markdown

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.

@BraisGabin BraisGabin added this to the 1.20.0 milestone Jan 25, 2022
@BraisGabin BraisGabin force-pushed the FullQualifiedNameGuesser branch from 376ac23 to b549bb1 Compare January 28, 2022 09:06
]
)
fun `all cases`(exclusion: String, annotation: String, shouldExclude: Boolean) {
val excluder = AnnotationExcluder(file, listOf(exclusion))
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 force-pushed the FullQualifiedNameGuesser branch from b549bb1 to b4c9bd6 Compare January 30, 2022 12:36
@BraisGabin BraisGabin force-pushed the FullQualifiedNameGuesser branch from b4c9bd6 to 7d35ecf Compare January 30, 2022 12:44
Comment thread detekt-psi-utils/api/detekt-psi-utils.api Outdated
Comment thread detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt Outdated
@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.

4 participants