feat(linter/no-extra-boolean-cast): implement auto-fixer #10682
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. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Instrumentation Performance ReportMerging #10682 will not alter performanceComparing Summary
|
DonIsaac
force-pushed
the
don/04-28-feat_linter/no-extra-boolean-cast_implement_auto-fixer
branch
from
504e2b6 to
c540774
Compare
Merge activity
|
WalkthroughThe updates enhance the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/oxc_linter/src/rules/eslint/no_extra_boolean_cast.rs (1)
120-150: Parameter naming drift – function still refers to logical operands
is_flagged_ctxandis_logical_ctxkeep the
enforce_for_logical_operandsidentifier even after the option was
renamed toenforce_for_inner_expressions. While purely cosmetic, this
is confusing and makes the code harder to follow. Please rename the
parameter in both functions to match the new terminology.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/oxc_linter/src/rules/eslint/no_extra_boolean_cast.rs(9 hunks)crates/oxc_macros/src/declare_oxc_lint.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/oxc_macros/src/declare_oxc_lint.rs (1)
crates/oxc_linter/src/rules/import/no_commonjs.rs (1)
is_conditional(112-129)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Conformance
- GitHub Check: Clippy
- GitHub Check: Test NAPI
- GitHub Check: Test (ubuntu-latest)
- GitHub Check: Test (macos-latest)
- GitHub Check: Test wasm32-wasip1-threads
- GitHub Check: Benchmark (parser)
- GitHub Check: Benchmark (transformer)
- GitHub Check: Benchmark (formatter)
- GitHub Check: Benchmark (lexer)
- GitHub Check: Benchmark (minifier)
- GitHub Check: Benchmark (codegen)
- GitHub Check: Benchmark (semantic)
- GitHub Check: Benchmark (isolated_declarations)
- GitHub Check: Build Linter Benchmark
🔇 Additional comments (2)
crates/oxc_linter/src/rules/eslint/no_extra_boolean_cast.rs (2)
64-74: Configuration parsing silently ignores object-form configs
from_configurationassumesvalueis an array and reads onlyvalue.get(0).
ESLint-style configs are typically objects ({ "enforceForInnerExpressions": true }).
If the caller passes an object directly, the option is ignored and the rule behaves as if the flag werefalse.Consider accepting both array and object forms to avoid surprising users:
let obj = value.get(0).unwrap_or(&value); // allow bare object let enforce_for_inner_expressions = obj .get("enforceForInnerExpressions") .or(obj.get("enforceForLogicalOperands")) .and_then(Value::as_bool) .unwrap_or(false);
212-224:⚠️ Potential issue
remove_double_notremoves too much for odd counts of!
remove_double_notblindly strips the first two negations it sees.
For!!!foothis yieldsfoo, not!fooas required, producing an
incorrect fix. A safer approach is to strip exactly two leading!
and leave the remainder untouched:fn remove_double_not<'a, 'b>(expr: &'b Expression<'a>) -> &'b Expression<'a> { - without_not(expr).and_then(|inner| without_not(inner)).unwrap_or(expr) + let mut current = expr; + for _ in 0..2 { + if let Some(next) = without_not(current) { + current = next; + } else { + return expr; // not a double-negation + } + } + current }Likely an incorrect or invalid review comment.
graphite-app
bot
force-pushed
the
don/04-28-feat_linter/no-extra-boolean-cast_implement_auto-fixer
branch
from
4b2d242 to
fb070c4
Compare
graphite-app
bot
deleted the
don/04-28-feat_linter/no-extra-boolean-cast_implement_auto-fixer
branch
No description provided.