feat(css_formatter): improve SCSS @each rule formatting#10202
Merged
Conversation
|
Contributor
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
Contributor
WalkthroughThis PR introduces a dedicated Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/biome_css_formatter/src/scss/auxiliary/each_header.rs (2)
26-26: 💤 Low valuePrefer
!bindings.is_empty()overlen() > 0.Minor nit: idiomatic Rust favours
!bindings.is_empty()for clarity.🔧 Suggested change
- if bindings.len() > 0 && is_direct_list_or_map_iterable(&iterable) { + if !bindings.is_empty() && is_direct_list_or_map_iterable(&iterable) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_formatter/src/scss/auxiliary/each_header.rs` at line 26, The condition uses bindings.len() > 0 which is non-idiomatic; change the check to use !bindings.is_empty() in the if expression that also calls is_direct_list_or_map_iterable(&iterable) so the line reads roughly: if !bindings.is_empty() && is_direct_list_or_map_iterable(&iterable) — update the reference to bindings, iterable and keep the rest of the logic intact.
69-88: 💤 Low valueSilent no-op for unexpected expression items warrants a defensive check.
The
_ => Ok(())branch silently succeeds without formatting anything. Given thatis_direct_list_or_map_iterableat line 26 already guards entry, this path should be unreachable under normal conditions. Consider adding adebug_assert!to catch unexpected states during development.🛡️ Defensive assertion suggestion
Some(AnyScssExpressionItem::ScssMapExpression(map)) => write!( f, [FormatOwnedWithRule::new( map, FormatScssEachIterableMap::new(self.bindings, self.in_token, node.syntax()) )] ), - _ => Ok(()), + _ => { + debug_assert!(false, "FormatScssEachIterable reached with non-list/map item"); + Ok(()) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_formatter/src/scss/auxiliary/each_header.rs` around lines 69 - 88, The match in FormatScssEachIterable::fmt_fields currently falls through to `_ => Ok(())`, silently doing nothing for unexpected expression kinds; add a defensive debug assertion to catch unreachable states during development by asserting the node shape returned by single_expression_item is one of the handled variants. Modify fmt_fields (impl FormatNodeRule<ScssExpression> for FormatScssEachIterable) to keep the existing arms for AnyScssExpressionItem::ScssListExpression and ::ScssMapExpression but replace the `_ => Ok(())` branch with a debug_assert!(matches!(single_expression_item(node), Some(AnyScssExpressionItem::ScssListExpression(_) | AnyScssExpressionItem::ScssMapExpression(_)))) (or debug_assert!(false, "...")) and then return Err or Ok(()) as before; reference function names: fmt_fields, single_expression_item, AnyScssExpressionItem, FormatScssEachIterableList, and FormatScssEachIterableMap so you update the exact match arm safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@crates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rs`:
- Around line 572-573: This change renames the named-field slots exposed to
GritQL for ScssEachAtRule (previously exposing bindings, iterable, block; now
exposing header and block) and is a user-facing breaking change—add a BREAKING
CHANGES entry in the release changelog and/or migration guide that documents the
rename and gives the migration pattern: update any GritQL patterns matching
ScssEachAtRule { bindings: $b, iterable: $i, ... } to ScssEachAtRule { header:
ScssEachHeader { bindings: $b, iterable: $i }, block: $blk } and mention that
patterns using the old top-level bindings/iterable will stop matching silently.
---
Nitpick comments:
In `@crates/biome_css_formatter/src/scss/auxiliary/each_header.rs`:
- Line 26: The condition uses bindings.len() > 0 which is non-idiomatic; change
the check to use !bindings.is_empty() in the if expression that also calls
is_direct_list_or_map_iterable(&iterable) so the line reads roughly: if
!bindings.is_empty() && is_direct_list_or_map_iterable(&iterable) — update the
reference to bindings, iterable and keep the rest of the logic intact.
- Around line 69-88: The match in FormatScssEachIterable::fmt_fields currently
falls through to `_ => Ok(())`, silently doing nothing for unexpected expression
kinds; add a defensive debug assertion to catch unreachable states during
development by asserting the node shape returned by single_expression_item is
one of the handled variants. Modify fmt_fields (impl
FormatNodeRule<ScssExpression> for FormatScssEachIterable) to keep the existing
arms for AnyScssExpressionItem::ScssListExpression and ::ScssMapExpression but
replace the `_ => Ok(())` branch with a
debug_assert!(matches!(single_expression_item(node),
Some(AnyScssExpressionItem::ScssListExpression(_) |
AnyScssExpressionItem::ScssMapExpression(_)))) (or debug_assert!(false, "..."))
and then return Err or Ok(()) as before; reference function names: fmt_fields,
single_expression_item, AnyScssExpressionItem, FormatScssEachIterableList, and
FormatScssEachIterableMap so you update the exact match arm safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0c9c506-1aeb-402b-b316-f273b31f6938
⛔ Files ignored due to path filters (14)
crates/biome_css_factory/src/generated/node_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_factory/src/generated/syntax_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_formatter/tests/specs/prettier/scss/atrule/each.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/scss/function/arbitrary-arguments.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/scss/interpolation/4294.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/scss/map/function-argument/function-argument-2.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/scss/map/function-argument/functional-argument.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/scss/at-rule/each.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/each.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/each.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_syntax/src/generated/kind.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/macros.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes_mut.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (15)
crates/biome_css_formatter/src/comments.rscrates/biome_css_formatter/src/generated.rscrates/biome_css_formatter/src/scss/auxiliary/each_header.rscrates/biome_css_formatter/src/scss/auxiliary/expression.rscrates/biome_css_formatter/src/scss/auxiliary/expression/each_iterable_list.rscrates/biome_css_formatter/src/scss/auxiliary/mod.rscrates/biome_css_formatter/src/scss/statements/each_at_rule.rscrates/biome_css_formatter/src/utils/mod.rscrates/biome_css_formatter/src/utils/scss_each.rscrates/biome_css_formatter/src/utils/scss_map_layout.rscrates/biome_css_formatter/tests/specs/scss/at-rule/each.scsscrates/biome_css_parser/src/syntax/scss/at_rule/each_at_rule.rscrates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rsxtask/codegen/css.ungramxtask/codegen/src/css_kinds_src.rs
💤 Files with no reviewable changes (1)
- crates/biome_css_formatter/src/scss/auxiliary/expression/each_iterable_list.rs
Merging this PR will not alter performance
Comparing Footnotes
|
15e9922 to
5807b19
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_css_formatter/src/scss/auxiliary/each_header.rs`:
- Around line 183-194: The current line-comment branch inserts hard_line_break()
between binding.node()?.format() and in_token.format(), which can produce
"$x\nin // comment ..."; instead move the break so the binding and in_token stay
together: in the fill.entry call that builds group(indent(format_args![...]))
concatenate binding.node()?.format() and in_token.format() (with a space) before
invoking hard_line_break(), then place hard_line_break() and finally
format_leading_comments(comment_owner) so the forced break happens after "in"
(i.e., before the iterable items) rather than between the binding and "in".
In `@crates/biome_css_formatter/tests/specs/scss/at-rule/each.scss`:
- Around line 46-66: Add a snapshot test for the same-line "in (" spacing
variant by inserting a new `@each` case that uses "in (" on the same line (e.g.
`@each` $element, $size in (h1:20px,h2:16px,h3:14px) { }) alongside the existing
`@each` examples; update the specs snapshot for the at-rule/each.scss tests so the
formatter is exercised for the `in (` branch and the new case is committed with
its snapshot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd44765d-15d1-417a-870e-2e808519d9ae
⛔ Files ignored due to path filters (14)
crates/biome_css_factory/src/generated/node_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_factory/src/generated/syntax_factory.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_formatter/tests/specs/prettier/scss/atrule/each.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/scss/function/arbitrary-arguments.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/scss/interpolation/4294.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/scss/map/function-argument/function-argument-2.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/prettier/scss/map/function-argument/functional-argument.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_formatter/tests/specs/scss/at-rule/each.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/error/scss/at-rule/each.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_parser/tests/css_test_suite/ok/scss/at-rule/each.scss.snapis excluded by!**/*.snapand included by**crates/biome_css_syntax/src/generated/kind.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/macros.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes.rsis excluded by!**/generated/**,!**/generated/**and included by**crates/biome_css_syntax/src/generated/nodes_mut.rsis excluded by!**/generated/**,!**/generated/**and included by**
📒 Files selected for processing (15)
crates/biome_css_formatter/src/comments.rscrates/biome_css_formatter/src/generated.rscrates/biome_css_formatter/src/scss/auxiliary/each_header.rscrates/biome_css_formatter/src/scss/auxiliary/expression.rscrates/biome_css_formatter/src/scss/auxiliary/expression/each_iterable_list.rscrates/biome_css_formatter/src/scss/auxiliary/mod.rscrates/biome_css_formatter/src/scss/statements/each_at_rule.rscrates/biome_css_formatter/src/utils/mod.rscrates/biome_css_formatter/src/utils/scss_each.rscrates/biome_css_formatter/src/utils/scss_map_layout.rscrates/biome_css_formatter/tests/specs/scss/at-rule/each.scsscrates/biome_css_parser/src/syntax/scss/at_rule/each_at_rule.rscrates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rsxtask/codegen/css.ungramxtask/codegen/src/css_kinds_src.rs
💤 Files with no reviewable changes (1)
- crates/biome_css_formatter/src/scss/auxiliary/expression/each_iterable_list.rs
✅ Files skipped from review due to trivial changes (9)
- crates/biome_css_formatter/src/utils/mod.rs
- crates/biome_css_formatter/src/scss/auxiliary/mod.rs
- crates/biome_css_parser/src/syntax/scss/at_rule/each_at_rule.rs
- xtask/codegen/src/css_kinds_src.rs
- crates/biome_css_formatter/src/scss/statements/each_at_rule.rs
- crates/biome_css_formatter/src/generated.rs
- crates/biome_css_formatter/src/comments.rs
- xtask/codegen/css.ungram
- crates/biome_grit_patterns/src/grit_target_language/css_target_language/generated_mappings.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_css_formatter/src/scss/auxiliary/expression.rs
- crates/biome_css_formatter/src/utils/scss_each.rs
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
1 participant
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.
Summary
Aligned
@eachformatting with Prettier for source-tightinspacing and direct map/list iterables.Test Plan