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

Verify at compile time that issue id matches rule name #4047

Merged
merged 6 commits into from
Oct 12, 2021

Conversation

marschwar
Copy link
Contributor

This relates to #4042 and adds a check that verifies that the issue ID matches the rule name.

@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

Merging #4047 (5bfaa43) into main (0a36aa9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #4047   +/-   ##
=========================================
  Coverage     83.45%   83.45%           
- Complexity     3181     3182    +1     
=========================================
  Files           465      465           
  Lines          9104     9104           
  Branches       1774     1774           
=========================================
  Hits           7598     7598           
  Misses          572      572           
  Partials        934      934           

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 0a36aa9...5bfaa43. Read the comment docs.

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.

Does this do the trick? Does core tests wait for all the rule modules to be compiled? testRuntimeOnly(projects.detektRules) does that?

@marschwar
Copy link
Contributor Author

Seems like it. I intentionally broke the build with e2d32db and the check failed.

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

I believe we should merge this, but at the same time I'd like those kind of checks to happen "earlier" in the rule lifecycle.

Specifically I believe we should update:

data class Issue(
    val id: String,
    val severity: Severity,
    val description: String,
    val debt: Debt
) {

to have a ctor that accepts a Rule as parameter. Or even we should remove the id from Issue entirely, as the Rule class is using it to expose the ruleId property:

final override val ruleId: RuleId get() = issue.id

So we're creating a tight couplign between the two classes, and we don't need to duplicate informations with the risk of having bugs like the one we just experienced.

@marschwar
Copy link
Contributor Author

Entirely agree. This is not more that a band-aid as this problem can easily be missed in the PR and the consequences are quite big.

@BraisGabin BraisGabin added the housekeeping Marker for housekeeping tasks and refactorings label Oct 12, 2021
@BraisGabin BraisGabin added this to the 1.19.0 milestone Oct 12, 2021
@BraisGabin BraisGabin merged commit 8b31b44 into detekt:main Oct 12, 2021
@marschwar marschwar deleted the check-rule-id branch January 24, 2022 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants