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

Add new id for the rules #7313

Merged
merged 3 commits into from
Jun 5, 2024
Merged

Add new id for the rules #7313

merged 3 commits into from
Jun 5, 2024

Conversation

BraisGabin
Copy link
Member

This PR changes some names. The first one is that Rule has a Name instead of an Id. A rule can no longer be identified by a static name.

The second one is to introduce a new id that can be different from Rule.Name.

And because now RuleInfo had a name and an id I renamed it to RuleInstance.

Naming is REALLY difficult so any help here is more than welcome.

This PR is part of #7263 but because that PR contains a lot of decissions I prefer to split it so we can talk about each one in different PRs but I recomment to read #7263 so you get the full context about why this change is needed.

Copy link

codecov bot commented May 19, 2024

Codecov Report

Attention: Patch coverage is 96.10390% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.31%. Comparing base (ac0ee12) to head (0225827).
Report is 2 commits behind head on main.

Files Patch % Lines
...otlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt 89.47% 0 Missing and 2 partials ⚠️
...arturbosch/detekt/core/suppressors/Suppressions.kt 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7313   +/-   ##
=========================================
  Coverage     84.31%   84.31%           
  Complexity     4149     4149           
=========================================
  Files           573      573           
  Lines         11888    11890    +2     
  Branches       2458     2458           
=========================================
+ Hits          10023    10025    +2     
  Misses          613      613           
  Partials       1252     1252           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BraisGabin
Copy link
Member Author

@detekt/maintainers I need reviews here. The PR is big but they are basically renames. The thing that I want you to review is if you agree with the names. Now that we are going to have multiple instances of the same rule we need "two" identifiers. The name of the rule (previously called id) and the id of the instance of that rule.

In general we should always use the new "id" but there are use cases for use the name, for example to link to the documentation.

Also, this PR renames RuleInfo to RuleInstance.

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.

LGTM 🚢

@BraisGabin BraisGabin merged commit 5b48024 into main Jun 5, 2024
21 checks passed
@BraisGabin BraisGabin deleted the new-id-on-ruleinstance branch June 5, 2024 09:51
@detekt-ci
Copy link
Collaborator

Warnings
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the detekt release notes.

Generated by 🚫 dangerJS against 0225827

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants