feat(css): enhance SCSS qualified name detection#9159
Conversation
|
Parser conformance results onjs/262
jsx/babel
markdown/commonmark
symbols/microsoft
ts/babel
ts/microsoft
|
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds parsing and formatting support for a new SCSS parent selector value ( Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/biome_css_parser/src/syntax/value/function.rs (1)
79-82: SCSS qualified function detection —n + 3offset is intentional but worth a comment.Per the PR objectives,
module.function(...)should be allowed butmodule.$function(...)should be rejected. Then + 3offset correctly matches only theident.ident(form (3 tokens before the paren), intentionally excludingident.$ident((which would needn + 4). A brief inline comment would make this intent clearer to future readers.📝 Suggested comment
(is_nth_at_identifier(p, n) && p.nth_at(n + 1, T!['('])) + // Only match `module.function(`, not `module.$function(` — the latter is rejected. || (is_nth_at_scss_qualified_name(p, n) && p.nth_at(n + 3, T!['(']))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_css_parser/src/syntax/value/function.rs` around lines 79 - 82, In is_nth_at_function, add a concise inline comment explaining that the (is_nth_at_scss_qualified_name(p, n) && p.nth_at(n + 3, T!['('])) branch intentionally uses n + 3 to match the three-token form ident.ident( (i.e., module.function(...)) and thereby exclude forms like ident.$ident( which would require n + 4); place the comment immediately before or on the same line as the n + 3 check so future readers understand the choice.
🤖 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/tests/specs/css/scss/declaration/parent-and-colon-values.scss`:
- Around line 2-5: The test currently uses a literal parent selector with
"&--primary" but doesn't exercise the interpolation case; update the input to
use interpolated selector "#{$self}--primary" (keeping the same $self: &; and
the nested rule body) so the formatter test mirrors the parser test and covers
the "#{...}" interpolation path for $self and the selector "#{$self}--primary".
In
`@crates/biome_css_parser/tests/css_test_suite/ok/scss/value/parent-and-colon-values.scss`:
- Around line 2-5: Add a test case exercising the interpolation of the saved
parent selector by including the selector form using #{$self}, e.g.
`#{$self}--primary`, alongside the existing `$self: &;` and `&--primary` entries
in the SCSS test file so the ScssParentSelectorValue/selector interpolation path
is exercised; update the test in
css_test_suite/ok/scss/value/parent-and-colon-values.scss to include the
interpolation variant and ensure the expected output covers the interpolated
selector.
In `@xtask/codegen/css.ungram`:
- Around line 2514-2522: parse_any_value() never constructs ScssColonValue
because there is no branch for is_at_scss_colon_value(); add a branch in
parse_any_value() to check is_at_scss_colon_value() and call a new
parse_scss_colon_value() that consumes the ':' token and returns the
ScssColonValue node (implement both is_at_scss_colon_value() and
parse_scss_colon_value() following the patterns used for ScssParentSelectorValue
and other simple value nodes), update any factory/formatter hookups if needed,
and run `just gen-grammar css` to regenerate artifacts after modifying the
.ungram file.
---
Nitpick comments:
In `@crates/biome_css_parser/src/syntax/value/function.rs`:
- Around line 79-82: In is_nth_at_function, add a concise inline comment
explaining that the (is_nth_at_scss_qualified_name(p, n) && p.nth_at(n + 3,
T!['('])) branch intentionally uses n + 3 to match the three-token form
ident.ident( (i.e., module.function(...)) and thereby exclude forms like
ident.$ident( which would require n + 4); place the comment immediately before
or on the same line as the n + 3 check so future readers understand the choice.
crates/biome_css_formatter/tests/specs/css/scss/declaration/parent-and-colon-values.scss
Show resolved
Hide resolved
crates/biome_css_parser/tests/css_test_suite/ok/scss/value/parent-and-colon-values.scss
Show resolved
Hide resolved
Merging this PR will not alter performance
Comparing Footnotes
|
e43d2f8 to
290736b
Compare
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@crates/biome_css_parser/tests/css_test_suite/ok/scss/value/parent-and-colon-values.scss`:
- Around line 1-6: Test coverage is missing the interpolation path that uses the
captured parent selector value; add a test that exercises interpolation like
using the $self variable assigned from & (e.g., $self: &;) inside the selector
and interpolating it with appended text (the #{ $self }--primary pattern) so the
parser's ScssParentSelectorValue handling is validated — update the existing
test file that defines $self and the nested selector (the code with $self: &;
and &--primary) to include a case where the selector is referenced via
interpolation (#{...}) to ensure the interpolation branch is parsed and
serialized correctly.
Summary
Improved SCSS parsing consistency by reusing qualified-name lookahead logic.
Support:
Test Plan