Skip to content

fix(noCommaOperator): remove vue v-for hack#10137

Merged
dyc3 merged 2 commits into
mainfrom
dyc3/v-for-rm-hack
Apr 27, 2026
Merged

fix(noCommaOperator): remove vue v-for hack#10137
dyc3 merged 2 commits into
mainfrom
dyc3/v-for-rm-hack

Conversation

@dyc3

@dyc3 dyc3 commented Apr 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

follow up to #8620

closes #9075

Test Plan

Docs

@changeset-bot

changeset-bot Bot commented Apr 27, 2026 •
edited
Loading

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: bc31ceb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dyc3 Graphite App

dyc3 commented Apr 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

This was referenced Apr 27, 2026
Merged
Merged
@github-actions github-actions Bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Apr 27, 2026
@dyc3 dyc3 marked this pull request as ready for review April 27, 2026 14:12
@dyc3 dyc3 changed the base branch from dyc3/v-for-nested-destructuring to graphite-base/10137 April 27, 2026 16:04
@dyc3 dyc3 force-pushed the graphite-base/10137 branch from 2819409 to ba1d8ea Compare April 27, 2026 16:23
@dyc3 dyc3 force-pushed the dyc3/v-for-rm-hack branch from 5112891 to 19c5acd Compare April 27, 2026 16:23
@dyc3 dyc3 changed the base branch from graphite-base/10137 to main April 27, 2026 16:23
@coderabbitai

coderabbitai Bot commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

This change removes the special case handling that prevented the noCommaOperator lint rule from executing in Vue template expressions. The conditional early-return and associated JsFileSource import check have been deleted, allowing the rule to run consistently across Vue templates rather than being skipped for template expressions.

Suggested reviewers

  • ematipico
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: removing a Vue v-for hack from the noCommaOperator rule.
Description check ✅ Passed The description references the follow-up issue (#8620) and the issue being closed (#9075), relating to the hack removal objective.
Linked Issues check ✅ Passed The PR removes the conditional early-return preventing noCommaOperator from running in Vue template expressions, directly addressing issue #9075's requirement to remove the hack.
Out of Scope Changes check ✅ Passed All changes are scoped to removing the Vue v-for hack from the noCommaOperator rule; no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dyc3/v-for-rm-hack

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]
coderabbitai Bot reviewed Apr 27, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/biome_js_analyze/src/lint/complexity/no_comma_operator.rs (1)

57-71: ⚠️ Potential issue | 🟠 Major

Please add regression snapshots for the Vue v-for false-positive path.

This changes lint behaviour in template expressions, but the PR does not include rule snapshots validating the fix and guarding against regressions. Please add tests that (at minimum) cover Vue v-for aliases and a true comma-operator violation in template context.

As per coding guidelines **/*.{rs,ts,tsx,js}: “All code changes MUST include appropriate tests … and bug fixes require tests that reproduce and validate the fix.”

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@crates/biome_js_analyze/src/lint/complexity/no_comma_operator.rs` around
lines 57 - 71, Add regression snapshot tests for the no_comma_operator rule to
cover the Vue v-for false-positive and a real comma-operator violation inside
template expressions: add test cases exercising the rule's run function (the
logic using ctx.query() / seq and the JsSequenceExpression / JsForStatement
parent checks) that include a Vue v-for alias pattern (e.g., template expression
with comma-separated aliases) which should not trigger, and a template
expression that legitimately uses the comma operator which should trigger;
create snapshots for both cases using the rule test harness so future changes to
run() will be guarded against regressions.
🤖 Prompt for all review comments with AI agents 
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@crates/biome_js_analyze/src/lint/complexity/no_comma_operator.rs`:
- Around line 57-71: Add regression snapshot tests for the no_comma_operator
rule to cover the Vue v-for false-positive and a real comma-operator violation
inside template expressions: add test cases exercising the rule's run function
(the logic using ctx.query() / seq and the JsSequenceExpression / JsForStatement
parent checks) that include a Vue v-for alias pattern (e.g., template expression
with comma-separated aliases) which should not trigger, and a template
expression that legitimately uses the comma operator which should trigger;
create snapshots for both cases using the rule test harness so future changes to
run() will be guarded against regressions.
ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bf587559-436d-4ff9-ae6d-6b1bef704f2f

📥 Commits

Reviewing files that changed from the base of the PR and between ba1d8ea and bc31ceb.

📒 Files selected for processing (1)
  • crates/biome_js_analyze/src/lint/complexity/no_comma_operator.rs

@codspeed-hq

codspeed-hq Bot commented Apr 27, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 58 untouched benchmarks
⏩ 196 skipped benchmarks1

Comparing dyc3/v-for-rm-hack (bc31ceb) with main (ba1d8ea)

Open in CodSpeed

Footnotes

  1. 196 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@dyc3 dyc3 merged commit 948c377 into main Apr 27, 2026
29 of 31 checks passed
@dyc3 dyc3 deleted the dyc3/v-for-rm-hack branch April 27, 2026 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@ematipico ematipico ematipico approved these changes

Assignees

No one assigned

Labels

A-Linter Area: linter L-JavaScript Language: JavaScript and super languages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

📎 Remove hack that disables noCommaOperator in vue template expressions

2 participants

@dyc3 @ematipico