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

Set DetektJvm task source with SourceDirectorySet instead of file list #4151

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

3flex
Copy link
Member

@3flex 3flex commented Oct 3, 2021

Fixes #4127

Unfortunately I can't say definitively why this fixes the problem, but I suspect it's somewhat related to this: gradle/gradle#3417 (comment)

Passing kotlinSourceSet.kotlin.files passed a list of files to use as the source for the task, while kotlinSourceSet.kotlin passes a SourceDirectorySet, so it's possible the filtering based on regex described in the comment I linked won't apply when a full file path is provided, but works when the source directory set is passed in instead.

@3flex 3flex changed the title Set source with SourceDirectorySet instead of file list Set DetektJvm task source with SourceDirectorySet instead of file list Oct 3, 2021
@codecov
Copy link

codecov bot commented Oct 3, 2021

Codecov Report

Merging #4151 (22ae103) into main (e05eec5) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #4151      +/-   ##
============================================
- Coverage     83.45%   83.44%   -0.01%     
+ Complexity     3185     3177       -8     
============================================
  Files           463      465       +2     
  Lines          9095     9104       +9     
  Branches       1768     1775       +7     
============================================
+ Hits           7590     7597       +7     
- Misses          571      572       +1     
- Partials        934      935       +1     
Impacted Files Coverage Δ
.../arturbosch/detekt/rules/style/UseIsNullOrEmpty.kt 54.66% <0.00%> (-2.67%) ⬇️
...bosch/detekt/rules/complexity/LongParameterList.kt 83.33% <0.00%> (-1.12%) ⬇️
...otlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt 80.95% <0.00%> (-0.30%) ⬇️
...ab/arturbosch/detekt/core/config/ValidateConfig.kt 100.00% <0.00%> (ø)
...tlab/arturbosch/detekt/rules/style/UseDataClass.kt 77.94% <0.00%> (ø)
...b/arturbosch/detekt/rules/naming/FunctionNaming.kt 100.00% <0.00%> (ø)
...tlab/arturbosch/detekt/rules/bugs/LateinitUsage.kt 90.00% <0.00%> (ø)
...sch/detekt/rules/style/UnnecessaryAbstractClass.kt 82.92% <0.00%> (ø)
...etekt/rules/style/FunctionOnlyReturningConstant.kt 94.87% <0.00%> (ø)
.../arturbosch/detekt/core/suppressors/Suppressors.kt 100.00% <0.00%> (ø)
... and 3 more

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 e05eec5...22ae103. Read the comment docs.

@chao2zhang
Copy link
Member

I guess SourceDirectorySet is a reactive/live data structure that it would be able to consider filetree operations like exclude.
kotlin.files, on the other hand, is a Set<File> which is definitely one time data structure.

@chao2zhang chao2zhang added this to the 1.19.0 milestone Oct 3, 2021
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.

Could we unit test this somehow?

@cortinico
Copy link
Member

I guess SourceDirectorySet is a reactive/live data structure that it would be able to consider filetree operations like exclude.
kotlin.files, on the other hand, is a Set<File> which is definitely one time data structure.

I believe that's exactly the case. But this still feels like a best guess fix. Are we able to verify it somehow (unit or manual testing)?

@chao2zhang
Copy link
Member

In my opinion, this fix should be in 1.19.0 - @3flex Would you have time to verify this in a repro project? I can help if you don't have time.

@3flex
Copy link
Member Author

3flex commented Nov 17, 2021

I tested manually before submitting the PR.

I'm working on adding some functional tests for the Gradle plugin but I'm finding odd issues with TestKit so it will take a bit more time to add an automated test.

If anyone wants to verify manually that might be enough to get it over the line...

@BraisGabin
Copy link
Member

I didn't check that the excludes work. But I tested it in my project and it doesn't introduce any regression as far as I can see.

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.

Running detekt in "type resolution" mode ignores "excludes" specified in gradle
4 participants