feat: add RuleOverwrite to rule#1546
Conversation
WalkthroughThe changes involve significant modifications to the configuration handling within the CLI, particularly focusing on the management of rules. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1546 +/- ##
==========================================
+ Coverage 72.31% 72.68% +0.36%
==========================================
Files 88 88
Lines 6018 6029 +11
==========================================
+ Hits 4352 4382 +30
+ Misses 1666 1647 -19 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
crates/cli/src/verify.rs (1)
60-60: LGTM! Consider adding a comment for clarity.The change from
NonetoDefault::default()aligns with the newRuleOverwritestructure and maintains consistency with similar updates in other files. This modification doesn't affect the overall logic of the function but may impact how rules are processed.Consider adding a brief comment explaining the purpose of
Default::default()in this context, especially if theRuleOverwritestruct has a non-trivial default implementation. This would improve code readability and maintainability.- let collections = &find_rules(arg.config.clone(), Default::default())?.0; + // Use default RuleOverwrite configuration + let collections = &find_rules(arg.config.clone(), Default::default())?.0;crates/cli/src/utils/rule_overwrite.rs (3)
12-16: Questioning the necessity of derivingDefaultforRuleOverwriteThe
#[derive(Default)]trait is added to theRuleOverwritestruct. Since all fields inside the struct are types that implementDefault, this is acceptable. However, ifRuleOverwriteis only instantiated via thenewmethod, theDefaulttrait may be unnecessary. Consider removing it unless there is a specific need for default instantiation elsewhere in the codebase.
Line range hint
36-73: Optimize cloning offilterin thenewmethodIn the
newmethod,filter: &Option<Regex>is cloned to initializerule_filter. SinceRegexis internally reference-counted, cloning is cheap. However, to avoid the unnecessary cloning of theOption, you could change the parameter to take ownership:filter: Option<Regex>. This way, you can directly assignrule_filter: filterwithout cloning.Apply this diff to modify the parameter and assignment:
-pub fn new(cli: &SeverityArg, filter: &Option<Regex>) -> Result<Self> { +pub fn new(cli: &SeverityArg, filter: Option<Regex>) -> Result<Self> { ... Ok(Self { default_severity, by_rule_id, - rule_filter: filter.clone(), + rule_filter: filter, })
102-116: Handle empty filter results with a more informative errorIn the
filter_rule_by_regexfunction, if no rules match the provided regex filter, an error is returned. Consider providing additional information or suggestions in the error message to help users understand why no rules were matched and how they might adjust their filter.Apply this diff to enhance the error message:
if selected.is_empty() { - Err(anyhow::anyhow!(EC::RuleNotFound(filter.to_string()))) + Err(anyhow::anyhow!(EC::RuleNotFound(format!( + "No rules matched the provided filter: {}. Please check the filter pattern or available rule IDs.", + filter + )))) } else { Ok(selected) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- crates/cli/src/config.rs (4 hunks)
- crates/cli/src/lsp.rs (1 hunks)
- crates/cli/src/scan.rs (2 hunks)
- crates/cli/src/utils/mod.rs (1 hunks)
- crates/cli/src/utils/rule_overwrite.rs (4 hunks)
- crates/cli/src/verify.rs (1 hunks)
- crates/config/src/rule_config.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (10)
crates/cli/src/utils/mod.rs (1)
11-11: LGTM. Verify usage ofRuleOverwritein the codebase.The new public export of
RuleOverwritealigns with the PR objectives. This change makes theRuleOverwriteentity accessible to other parts of the codebase, which is likely intentional for the new rule management system.To ensure this change is properly integrated, please verify its usage across the codebase:
crates/cli/src/scan.rs (3)
20-20: LGTM: New import added correctly.The
RuleOverwriteimport fromcrate::utilsis appropriately placed and grouped with other imports from the same module.
Line range hint
1-424: Overall assessment: Good improvements to rule management.The changes in this file are focused and align well with the PR objectives. The introduction of
RuleOverwriteand its usage in thetry_newmethod ofScanWithConfigrepresent a positive step towards more structured and flexible rule management. The modifications are consistent with the PR summary and should enhance the overall functionality of the CLI configuration handling.
124-125: LGTM: Improved rule management withRuleOverwrite.The introduction of
RuleOverwriteenhances the structure and potentially the flexibility of rule management. This change aligns well with the refactoring mentioned in the PR summary.To ensure consistency across the codebase, please run the following script:
crates/config/src/rule_config.rs (1)
211-215: LGTM! EnhancesRuleConfig<L>usability.The implementation of
DerefMutforRuleConfig<L>is correct and follows Rust conventions. This addition complements the existingDerefimplementation, providing both immutable and mutable access to the innerSerializableRuleConfig<L>. This enhancement improves the usability ofRuleConfig<L>by allowing direct mutable access to its inner configuration.crates/cli/src/config.rs (5)
1-2: Necessary imports added correctlyThe imports of
RuleOverwriteandRuleTraceare appropriately added to support the new rule management functionality.
90-93: Function call toread_directory_yamlupdated correctlyThe call to
read_directory_yamlnow includes therule_overwriteparameter, aligning with the updated function signature.
Line range hint
155-159: Updatedread_directory_yamlfunction to includerule_overwriteThe function
read_directory_yamlnow acceptsrule_overwrite: RuleOverwriteas a parameter, which reflects the new structured approach to rule management.
184-184: Applyingrule_overwriteto process configurationsThe invocation of
rule_overwrite.process_configs(configs)?correctly integrates the new rule overwriting logic into the configuration processing.
86-89: Verify all calls tofind_rulesare updated with the new parameterThe function
find_rulesnow includes therule_overwrite: RuleOverwriteparameter. Please ensure that all calls to this function throughout the codebase are updated to match the new signature.You can use the following script to locate any calls to
find_rulesthat may not have been updated:Expected result: No matches should be found. If any matches are found, those calls need to be updated to include the
rule_overwriteparameter.
Summary by CodeRabbit
New Features
RuleOverwriteutility for enhanced rule management in configurations.Bug Fixes
Documentation
Refactor
Style
Optiontypes.