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

Extend AnnotationExcluder to catch fully qualified annotations #2361

Merged

Conversation

cortinico
Copy link
Member

Currently AnnotationExcluder is not handling usages of fully qualified annotations (eg. @dagger.Module). This is causing the failure reported in #2360.
I'm extending it to catch both simple and fully qualified annotations.

Fixes #2360

@codecov-io
Copy link

codecov-io commented Feb 19, 2020

Codecov Report

Merging #2361 into master will increase coverage by 0.02%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2361      +/-   ##
============================================
+ Coverage     82.81%   82.83%   +0.02%     
  Complexity     2162     2162              
============================================
  Files           350      350              
  Lines          6056     6059       +3     
  Branches       1110     1110              
============================================
+ Hits           5015     5019       +4     
  Misses          469      469              
+ Partials        572      571       -1
Impacted Files Coverage Δ Complexity Δ
...gitlab/arturbosch/detekt/api/AnnotationExcluder.kt 50% <71.42%> (+27.77%) 5 <4> (ø) ⬇️

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 9f64c26...f7d9052. Read the comment docs.

@cortinico
Copy link
Member Author

Don't really know why UnusedPrivateClassSpec > does not report (companion-)object/named-dot references is failing only for Java8 on Travis 🧐

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's failing because the code snippet doesn't compile. We only compile them in Travis Java8 (performance reasons). You can test it with: ./gradlew :detekt-rules:test -Pcompile-test-snippets=true

I think that for really fix this issue we need type solving but this is better that we have for sure.

@cortinico
Copy link
Member Author

It's failing because the code snippet doesn't compile. We only compile them in Travis Java8 (performance reasons). You can test it with: ./gradlew :detekt-rules:test -Pcompile-test-snippets=true

Thanks for the clarification. Still, UnusedPrivateClassSpec is unrelated to my change 🧐

I think that for really fix this issue we need type solving but this is better that we have for sure.

Yeah agree.

@BraisGabin
Copy link
Member

Oh, yes, I didn't noticed that. It's strange, master is passing. Can you rebase your branch with the current master? I think that'll solve the problem

@cortinico cortinico force-pushed the fix-2360-fully-qualified-annotation-excluder branch from 8eeda07 to 8b1c8f3 Compare February 20, 2020 14:42
@cortinico
Copy link
Member Author

I think that'll solve the problem

Nope is still failing :/

@schalkms
Copy link
Member

@cortinico I had the same problem in #2350.
I just haven’t had the time so far to look into it.

On jdk8 we additionally compile the provided code snippets in detekt’s unit tests through Jsr223 in order to validate it, because there were problems with invalid Kotlin code in the past. That’s the reason why this PR only fails on this single build agent.

I suspect this error is because of one of the following reasons:

  • Using 8 compile engines for compiling around +1000 test snippets is not enough anymore.
  • The error mentions something about coroutines. There might be problems with the previously introduced coroutine ruleset test cases.

@schalkms
Copy link
Member

@cortinico btw thanks for your fast fixes. You helped us out multiple times. Thanks for that! 🙂

@schalkms schalkms merged commit 9fa2eb0 into detekt:master Feb 20, 2020
@arturbosch arturbosch added this to the 1.6.0 milestone Feb 21, 2020
@cortinico cortinico deleted the fix-2360-fully-qualified-annotation-excluder branch June 13, 2020 20:40
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.

UnnecessaryAbstractClass excludeAnnotatedClasses not working
5 participants