-
Notifications
You must be signed in to change notification settings - Fork 258
refactor: Add basic transformations for check, smells and build commands #2493
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
base: main
Are you sure you want to change the base?
Conversation
|
No issue mentions found. Please mention an issue in the pull request description. Use GitHub automation to close the issue when a PR is merged |
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.
Pull Request Overview
This pull request refactors the issue transformation system by:
- Changing
transform_batchsignature to accept a reference (&Vec<Issue>) instead of taking ownership - Moving shared transformation logic (
IssueMuter, triage handling, ignore rules) into a newBasicTransformationstrait inqlty-analysis - Moving
source_readerandissue_mutermodules fromqlty-checktoqlty-analysisfor better code organization - Applying basic transformations consistently across
check,build, andsmellscommands - Updating test expectations to reflect new triage behavior affecting issue ordering
Reviewed Changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| qlty-config/src/config/issue_transformer.rs | Updated trait and tests to use reference parameter |
| qlty-check/src/report.rs | Implemented BasicTransformations trait |
| qlty-check/src/planner.rs | Removed duplicate triage/muter logic now in BasicTransformations |
| qlty-check/src/lib.rs | Removed modules moved to qlty-analysis |
| qlty-check/src/llm/fixer.rs | Updated signature to match trait |
| qlty-check/src/executor.rs | Updated call site to pass reference |
| qlty-analysis/src/basic_transformations.rs | New trait centralizing transformation logic |
| qlty-analysis/src/source_reader.rs | Moved from qlty-check |
| qlty-analysis/src/issue_muter.rs | Moved from qlty-check with minor formatting updates |
| qlty-analysis/src/workspace_reader.rs | New wrapper for workspace-level file reading |
| qlty-analysis/src/report.rs | Implemented BasicTransformations trait with commented-out duplicate code |
| qlty-cli/src/commands/*.rs | Applied basic transformations to reports |
| Test files | Updated expectations for new behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| fn transform_batch(&self, issues: Vec<Issue>) -> Vec<Issue> { | ||
| fn transform_batch(&self, issues: &Vec<Issue>) -> Vec<Issue> { |
Copilot
AI
Oct 29, 2025
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 parameter type &Vec<Issue> should be &[Issue] instead. Using a slice reference is more idiomatic in Rust as it's more flexible (accepts both Vec and array references) and is the standard convention for borrowed sequences.
| fn transform_batch(&self, issues: &Vec<Issue>) -> Vec<Issue> { | |
| fn transform_batch(&self, issues: &[Issue]) -> Vec<Issue> { |
qlty-check/src/llm/fixer.rs
Outdated
|
|
||
| impl IssueTransformer for Fixer { | ||
| fn transform_batch(&self, issues: Vec<Issue>) -> Vec<Issue> { | ||
| fn transform_batch(&self, issues: &Vec<Issue>) -> Vec<Issue> { |
Copilot
AI
Oct 29, 2025
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 parameter type &Vec<Issue> should be &[Issue] instead. Using a slice reference is more idiomatic in Rust as it's more flexible (accepts both Vec and array references) and is the standard convention for borrowed sequences.
| fn transform_batch(&self, issues: &Vec<Issue>) -> Vec<Issue> { | |
| fn transform_batch(&self, issues: &[Issue]) -> Vec<Issue> { |
qlty-analysis/src/report.rs
Outdated
| // }); | ||
| // } | ||
| // } | ||
|
|
||
| // triages | ||
| // } | ||
|
|
||
| // pub fn apply_basic_issue_transformations( | ||
| // &mut self, | ||
| // workspace_root: &Path, | ||
| // qlty_config: &QltyConfig, | ||
| // ) { | ||
| // // Remove issues without location | ||
| // let transformers = self.compute_transformers(workspace_root, qlty_config); | ||
| // for transformer in transformers { | ||
| // self.transform_issues(transformer); | ||
| // } | ||
| // } | ||
|
|
Copilot
AI
Oct 29, 2025
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.
Remove commented-out code. This large block of commented code (lines 37-106) appears to be duplicate logic that was moved to BasicTransformations trait. Commented code should be removed to improve maintainability; the old implementation can be retrieved from version control if needed.
| // }); | |
| // } | |
| // } | |
| // triages | |
| // } | |
| // pub fn apply_basic_issue_transformations( | |
| // &mut self, | |
| // workspace_root: &Path, | |
| // qlty_config: &QltyConfig, | |
| // ) { | |
| // // Remove issues without location | |
| // let transformers = self.compute_transformers(workspace_root, qlty_config); | |
| // for transformer in transformers { | |
| // self.transform_issues(transformer); | |
| // } | |
| // } |
1 new issue
|
|
Diff Coverage for ubuntu-latest: The code coverage on the diff in this pull request is 95.7%. Total Coverage for ubuntu-latest: This PR will decrease coverage by 0.03%. File Coverage Changes
🛟 Help
|
|
Diff Coverage for macos-15: The code coverage on the diff in this pull request is 95.7%. Total Coverage for macos-15: This PR will decrease coverage by 0.03%. File Coverage Changes
🛟 Help
|
No description provided.