-
-
Notifications
You must be signed in to change notification settings - Fork 784
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
Run detekt with type resolution analysis on CI #3015
Conversation
detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/FindingsAssertions.kt
Outdated
Show resolved
Hide resolved
@@ -83,7 +83,7 @@ private class TestMultiRule(config: Config) : MultiRule() { | |||
} | |||
} | |||
|
|||
private abstract class AbstractRule(config: Config) : Rule(config) { | |||
private open class AbstractRule(config: Config) : Rule(config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 isn't this a false positive? I mean, we don't want any instance of AbstractRule
so mark it as abstract
seems like a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rule could additionally check if any inherited members used in it's members.
Or a simpler extends Something
check for not requiring type resolution.
@@ -14,6 +14,7 @@ import org.jetbrains.kotlin.psi.KtExpression | |||
/** | |||
* Rule to detect empty blocks of code. | |||
*/ | |||
@Suppress("detekt.UnnecessaryAbstractClass") // we really do not want instances of this class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should rethink this rule...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the only real purpose of this class is to share some logic, which could be done via composition.
It's methods also do two things addFindingIfBlockExprIsEmpty
check and add xD
java-version: 11 | ||
|
||
- name: Run analysis | ||
run: ./gradlew detektMain detektTest --build-cache --parallel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can change now the pre merge workflow so it doesn't runs detekt each time, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do this than only in the pre-merge.yml so it is run locally.
But do we safe so much time? detekt without type resolution is fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, but yes. Just one CI job for quality is a nicer separation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the idea is that. if intellij doesn't remove a unused import I don't want all the checks in red.
Codecov Report
@@ Coverage Diff @@
## master #3015 +/- ##
============================================
- Coverage 79.53% 79.15% -0.38%
- Complexity 2532 2543 +11
============================================
Files 431 434 +3
Lines 7637 7714 +77
Branches 1447 1465 +18
============================================
+ Hits 6074 6106 +32
- Misses 795 822 +27
- Partials 768 786 +18
Continue to review full report at Codecov.
|
…indingsAssertions.kt
7719034
to
34a294f
Compare
No description provided.