Conversation
github-actions
bot
added
A-linter
CodSpeed Performance ReportMerging #17479 will not alter performanceComparing Summary
Footnotes
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the from_configuration method across all lint rules to return a Result type instead of Self, enabling better error handling for configuration parsing failures in the future.
Key changes:
- Updated the
Ruletrait'sfrom_configurationsignature to returnResult<Self, serde_json::error::Error> - Modified all rule implementations to wrap their return values in
Ok() - Updated all call sites to unwrap the Result
Reviewed changes
Copilot reviewed 235 out of 235 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/oxc_linter/src/rule.rs | Updated the Rule trait definition and documentation to use Result return type |
| tasks/rulegen/template.txt | Updated template for new rules to use Result return type |
| crates/oxc_macros/src/declare_all_lint_rules.rs | Modified macro to propagate errors with ? operator |
| crates/oxc_linter/src/config/rules.rs | Added .unwrap() call when creating rules from configuration |
| crates/oxc_linter/src/tester.rs | Added .unwrap() call in test rule configuration |
| crates/oxc_linter/src/rules/**/*.rs | Updated 200+ individual rule implementations to return Ok(...) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
graphite-app
bot
force-pushed
the
pass-self-to-default-rule-config
branch
from
505e41e to
3c7ad0a
Compare
…om_configuration.
Fixes some issues, but not all of them yet.
connorshea
force-pushed
the
from-config-result
branch
from
5f07678 to
2b615be
Compare
camc314
approved these changes
Dec 30, 2025
camc314
approved these changes
Dec 30, 2025
graphite-app bot
pushed a commit
that referenced
this pull request
Jan 16, 2026
…t/no-hooks and eslint/no-return-assign rules. (#17600) AI Disclosure: The changes to `config_builder.rs` and DefaultRuleConfig were heavily assisted with AI (Copilot + Raptor mini + Claude Opus 4.5), but generally seem reasonable to me. I went through a few iterations to get a simpler implementation of the changeset and I'm relatively happy with where we're at now. I am unsure about how performant it is for a large/complex configuration, though. Closes #17506. Follow-up on #17479. Basically, this updates two rules (`jest/no-hooks` and `eslint/no-return-assign`) to surface errors when the user provides invalid configuration options. This will surface all config errors it finds and then bail without doing any linting. This will hard-error and not continue linting when it hits an invalid config object. There are valid arguments for not doing this and instead just logging a warning and falling back to default values, but I think this is probably the correct option? I have added direct tests to the rule files themselves for valid/invalid config options, it may be a good idea to update the Tester to accept array of invalid and valid config shapes, but that's something we can figure out later. I've also added some snapshot tests to ensure that we have snapshots of what it looks like when an invalid config option is hit, and can ensure we don't regress on this behavior in the future.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Built on top of #17478.
Based on the changes I experimented with in #17199, and the suggestion given by camc in that PR.
AI Disclosure: This was done almost entirely via find+replace and manual updates to fix individual rules. I did use AI for the change in 0abc981 since I had trouble figuring out a solution for that.
It may be easier to review this by going through it commit-by-commit.
Ran ecosystem CI on this branch and it looks like it works fine, no new regressions https://github.com/oxc-project/oxc-ecosystem-ci/actions/runs/20590908980