refactor(linter): optimize merge method and clarify precedence semantics #15786
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
- Revert merge signature to take ownership instead of reference - Update call site to pass ownership - Avoid unnecessary clones by using owned values - Expand comment with example showing merge priority Co-authored-by: camc314 <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Oxlintrc::merge method to improve performance and clarify merge semantics based on post-merge review feedback.
- Optimized
mergemethod signature to take ownership ofotherparameter instead of borrowing - Removed unnecessary clones of owned fields (
categoriesandoverrides) - Enhanced documentation with concrete example showing precedence semantics (self wins over other)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/oxc_linter/src/config/oxlintrc.rs | Updated merge method signature to take ownership and removed redundant clones; expanded documentation with precedence example |
| crates/oxc_linter/src/config/config_builder.rs | Updated call site to pass owned value instead of reference to match new signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// ``` | ||
| /// // If self has: { "rules": { "no-console": "error" } } | ||
| /// // and other has: { "rules": { "no-console": "warn", "no-debugger": "error" } } | ||
| /// // Result will be: { "rules": { "no-console": "error", "no-debugger": "error" } } | ||
| /// // (self's "no-console" setting wins) | ||
| /// ``` |
There was a problem hiding this comment.
The code block in this documentation example should specify a language tag or use ignore to prevent rustdoc from attempting to compile it as a Rust test. The current format will cause doc test failures.
Suggested fix:
/// # Example
///
/// ```ignore
/// // If self has: { "rules": { "no-console": "error" } }
/// // and other has: { "rules": { "no-console": "warn", "no-debugger": "error" } }
/// // Result will be: { "rules": { "no-console": "error", "no-debugger": "error" } }
/// // (self's "no-console" setting wins)
/// ```
There was a problem hiding this comment.
@copilot fix this, what's there currently doesn't really make sense. You've put a code block, with just comments inside. Perhaps use inline code block for just the json? or something else?
There was a problem hiding this comment.
Fixed in 1857911. Replaced the code block with inline code blocks for the JSON examples to make the documentation clearer.
Replace code block with only comments with inline code blocks for JSON examples to make the documentation clearer and prevent rustdoc errors. Co-authored-by: camc314 <[email protected]>
Addresses post-merge review feedback on #14939.
Changes
Oxlintrc::mergesignature: Take ownership ofotherparameter instead of borrowing, avoiding unnecessary clone since caller owns the valueother.categories,other.overrides)selfwins when both configs define the same propertyPreviously confusing which config took precedence; documentation now makes the behavior explicit and clear.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.