chore: combine attribute impl#9950
Conversation
|
WalkthroughThis change refactors HTML attribute parsing helpers in the Biome HTML syntax module. The implementation of five extension methods— 🚥 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)
Comment |
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_html_syntax/src/attribute_ext.rs`:
- Around line 12-17: The HtmlString arm currently turns a missing/invalid token
into Some("") by calling unwrap_or_default; instead preserve the absence by
returning None when value_token() is None. Replace the unwrap_or_default usage
in the Self::HtmlString branch so that you map the token with
value_token().map(|token| inner_string_text(&token).into()) and let that Option
propagate (i.e., remove the unwrap_or_default), ensuring the expression yields
None when the token is missing.
- Around line 7-66: Add unit/integration tests that exercise the moved helpers:
AnyHtmlAttributeInitializer::string_value,
AnyHtmlAttributeInitializer::as_static_value (where applicable),
HtmlAttributeList::find_by_name, HtmlAttribute::value,
HtmlAttributeName::token_text and token_text_trimmed; cover both positive cases
(attributes with plain strings, quoted strings, single-text expressions, and
name lookup matches) and negative cases (missing initializer, non-string
initializers, missing attribute name matches, and tokens that require trimming),
and use snapshot/assertion-based checks to lock the current behavior after the
refactor.
🪄 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: 24ca8afb-f8da-4a39-abbc-0c54e25d625b
📒 Files selected for processing (3)
crates/biome_html_syntax/src/attr_ext.rscrates/biome_html_syntax/src/attribute_ext.rscrates/biome_html_syntax/src/lib.rs
💤 Files with no reviewable changes (2)
- crates/biome_html_syntax/src/lib.rs
- crates/biome_html_syntax/src/attr_ext.rs
| impl AnyHtmlAttributeInitializer { | ||
| /// Returns the string value of the attribute, if available, without quotes. | ||
| pub fn string_value(&self) -> Option<Text> { | ||
| match self { | ||
| Self::HtmlAttributeSingleTextExpression(text) => text.expression().ok()?.string_value(), | ||
| Self::HtmlString(string) => Some( | ||
| string | ||
| .value_token() | ||
| .map(|token| inner_string_text(&token).into()) | ||
| .unwrap_or_default(), | ||
| ), | ||
| } | ||
| } | ||
|
|
||
| pub fn as_static_value(&self) -> Option<StaticValue> { | ||
| match self { | ||
| Self::HtmlAttributeSingleTextExpression(_) => None, | ||
| Self::HtmlString(value) => Some(StaticValue::String(value.value_token().ok()?)), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl HtmlAttributeList { | ||
| pub fn find_by_name(&self, name_to_lookup: &str) -> Option<HtmlAttribute> { | ||
| self.iter().find_map(|attribute| { | ||
| if let AnyHtmlAttribute::HtmlAttribute(attribute) = attribute | ||
| && let Ok(name) = attribute.name() | ||
| && name.value_token().ok()?.text_trimmed() == name_to_lookup | ||
| { | ||
| return Some(attribute); | ||
| } | ||
| None | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl HtmlAttribute { | ||
| /// Extracts the value from an attribute's initializer. | ||
| /// | ||
| /// Returns `None` if the attribute has no initializer or the value cannot be extracted. | ||
| pub fn value(&self) -> Option<Text> { | ||
| self.initializer() | ||
| .and_then(|init| init.value().ok()) | ||
| .and_then(|value| value.string_value()) | ||
| } | ||
| } | ||
|
|
||
| impl HtmlAttributeName { | ||
| /// Returns the token text of the attribute name. | ||
| pub fn token_text(&self) -> Option<TokenText> { | ||
| self.value_token().ok().map(|token| token.token_text()) | ||
| } | ||
|
|
||
| /// Returns the trimmed token text of the attribute name. | ||
| pub fn token_text_trimmed(&self) -> Option<TokenText> { | ||
| self.value_token() | ||
| .ok() | ||
| .map(|token| token.token_text_trimmed()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add coverage for the moved attribute helper APIs.
This refactor moves core helper behaviour, but no accompanying tests are included in this PR. Even for file consolidation, we should lock behaviour with tests for string_value, find_by_name, value, and token_text(_trimmed) paths.
As per coding guidelines: **/*.{rs,ts,tsx,js}: “All code changes MUST include appropriate tests: lint rules require snapshot tests in 'tests/specs/{group}/{rule}/', formatters require snapshot tests with valid/invalid cases, parsers require test files covering valid and error cases, 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_html_syntax/src/attribute_ext.rs` around lines 7 - 66, Add
unit/integration tests that exercise the moved helpers:
AnyHtmlAttributeInitializer::string_value,
AnyHtmlAttributeInitializer::as_static_value (where applicable),
HtmlAttributeList::find_by_name, HtmlAttribute::value,
HtmlAttributeName::token_text and token_text_trimmed; cover both positive cases
(attributes with plain strings, quoted strings, single-text expressions, and
name lookup matches) and negative cases (missing initializer, non-string
initializers, missing attribute name matches, and tokens that require trimming),
and use snapshot/assertion-based checks to lock the current behavior after the
refactor.
| Self::HtmlString(string) => Some( | ||
| string | ||
| .value_token() | ||
| .map(|token| inner_string_text(&token).into()) | ||
| .unwrap_or_default(), | ||
| ), |
There was a problem hiding this comment.
Avoid masking missing/invalid string tokens with an empty default.
Line 16 currently converts token access failures into Some(""), which can blur parse-error states and “empty value” states. Returning None here is safer and aligns with the method contract (“if available”).
Suggested fix
- Self::HtmlString(string) => Some(
- string
- .value_token()
- .map(|token| inner_string_text(&token).into())
- .unwrap_or_default(),
- ),
+ Self::HtmlString(string) => string
+ .value_token()
+ .ok()
+ .map(|token| inner_string_text(&token).into()),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Self::HtmlString(string) => Some( | |
| string | |
| .value_token() | |
| .map(|token| inner_string_text(&token).into()) | |
| .unwrap_or_default(), | |
| ), | |
| Self::HtmlString(string) => string | |
| .value_token() | |
| .ok() | |
| .map(|token| inner_string_text(&token).into()), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_html_syntax/src/attribute_ext.rs` around lines 12 - 17, The
HtmlString arm currently turns a missing/invalid token into Some("") by calling
unwrap_or_default; instead preserve the absence by returning None when
value_token() is None. Replace the unwrap_or_default usage in the
Self::HtmlString branch so that you map the token with value_token().map(|token|
inner_string_text(&token).into()) and let that Option propagate (i.e., remove
the unwrap_or_default), ensuring the expression yields None when the token is
missing.
Merging this PR will not alter performance
Comparing Footnotes
|
| string | ||
| .value_token() | ||
| .map(|token| inner_string_text(&token).into()) | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
| .unwrap_or_default(), |
There was a problem hiding this comment.
I think that does change a bit of logic, which is perhaps out of scope for this PR. It seems that it currently always returns a Some() then if there's not inner text it just returns an Some(""). But yeah could change some stuff around
Summary
Seems like there were 2 files for attribute implementation extensions, combined them
Test Plan
Docs