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
Move PathFilters
from detekt-api to detekt-utils
#6866
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6866 +/- ##
=========================================
Coverage 84.75% 84.75%
Complexity 3996 3996
=========================================
Files 577 577
Lines 12133 12133
Branches 2486 2486
=========================================
Hits 10283 10283
Misses 624 624
Partials 1226 1226 ☔ View full report in Codecov by Sentry. |
57b437a
to
dbdf680
Compare
detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt
Outdated
Show resolved
Hide resolved
Thinking about this, I don't think the includes/excludes belongs in detekt-tooling at all, and it makes sense to remove them from As an example, the Gradle plugin doesn't pass exclude rules. It already filters and just sends the file list to the CLI. I think it makes more sense to move the path filters to CLI, and remove includes/excludes from tooling and bring everything up a level. What do you think? |
dbdf680
to
3e30b4c
Compare
That you are completely right. I'm moving this PR to draft to block it and I'll create another PR removing includes/excludes from I like to move logic from rules to core. And then from core to clients. The rules and the core are complex enough already. |
3e30b4c
to
1508450
Compare
1508450
to
f931130
Compare
I rebased this on top of #7009. Now instead of moving I'm not a big fan of |
PathFilters
from detekt-api to detekt-utils
f931130
to
59ed596
Compare
59ed596
to
65a2ae2
Compare
This PR changed a bit since the last review. @3flex can you take a look again? I see two options here.
Neither of those seems like a bad idea to me. |
Completely agree - it's useful though unless someone comes up with a better idea. LGTM after merge conflict is addressed. |
7805f53
to
9360ab5
Compare
Also see #6814
We had a little mess around this class. I splitted some tests and move them to the correct locations and then movePathFilters
to:detekt-tooling
.We have far too much "utils" subprojects and I'm not 100% sure the responsability of which one, but this one seems the correct one. We don't want this at:detekt-api
because it is only used by:detekt-core
and by:detekt-cli
. And it would be really nice to move it to:detekt-core
but, as I said it is also used by:detekt-cli
.If someone create some documentation about which is the responsabilities of each subproject it would be great.Move
PathFilters
to:detekt-utils
. This class is used ondetekt-cli
anddetekt-core
but we don't want it to be part of our public API so I'm moving it to:detekt-utils
.Waiting for #7009