Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(css_semantic): improve CSS semantic model structure and API #4908

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

togami2864
Copy link
Contributor

Summary

  • Use CssSyntaxNode instead of a String so that the model has more flexibility and extensibility
  • Fixed the lint rule that already used the model

Test Plan

CI should pass

@togami2864 togami2864 self-assigned this Jan 18, 2025
@github-actions github-actions bot added A-Linter Area: linter L-CSS Language: CSS labels Jan 18, 2025
Comment on lines +33 to +45
// #[cfg(test)]
// mod tests {
// use biome_css_parser::parse_css;
// use biome_css_parser::CssParserOptions;
// use biome_rowan::TextRange;

// #[test]
// fn test_simple_ruleset() {
// let parse = parse_css(
// r#"p {
// font-family: verdana;
// font-size: 20px;
// }"#,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temporarily disable the tests. I will make another PR to split them into another module.

@togami2864 togami2864 changed the title Feat/change css model struct refactor(css_semantic): Improve CSS semantic model structure and API Jan 18, 2025
@togami2864 togami2864 changed the title refactor(css_semantic): Improve CSS semantic model structure and API refactor(css_semantic): improve CSS semantic model structure and API Jan 18, 2025
Copy link

codspeed-hq bot commented Jan 18, 2025

CodSpeed Performance Report

Merging #4908 will improve performances by 11.35%

Comparing togami2864:feat/change-css-model-struct (909e0fd) with main (738ad51)

Summary

⚡ 3 improvements
✅ 92 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
css_analyzer[bootstrap_18416142857265205439.css] 219.5 ms 204.6 ms +7.31%
css_analyzer[bulma_5641719244145477318.css] 624.5 ms 579 ms +7.85%
css_analyzer[foundation_11602414662825430680.css] 149.6 ms 134.4 ms +11.35%

},
range: node.text_range(),
value: CssValue { node: value },
Copy link
Member

Choose a reason for hiding this comment

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

imho: can be change to property_value as it aligns well with property_name?

Suggested change
value: CssValue { node: value },
value: CssValue { node: property_value },

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Way better! There are still few methods that allocate strings, which we should rework

crates/biome_css_semantic/src/semantic_model/model.rs Outdated Show resolved Hide resolved
Comment on lines 271 to 273
pub fn text(&self) -> String {
self.node.text_trimmed().to_string()
}
Copy link
Member

Choose a reason for hiding this comment

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

No allocations. If the caller needs a String, they can do it. We should return a SyntaxText from here

crates/biome_css_semantic/src/semantic_model/model.rs Outdated Show resolved Hide resolved
crates/biome_css_semantic/Cargo.toml Outdated Show resolved Hide resolved
@ematipico
Copy link
Member

@togami2864

I pushed some changes in this PR here 909e0fd (#4908). Here's some feedback for you, in the future:

  • Avoid using "raw syntax" nodes whenever possible, because the code breaks without noticing if grammar changes. Always cast nodes to typed nodes, so you have a safer API to navigate, and if the grammar changes, the compiler will error.
  • Never use raw nodes inside types. I changed CssProperty to use the correct identifiers. This allowed us to create the value() function, which allowed us not to allocate a String, which was only used for some internal check.
  • I couldn't change CssValue to use typed nodes, however I urge you to change whenever you can, because it doesn't give us enough information of the expected value. Plus, if the grammar changes, the code that relies on CssValue could break

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-CSS Language: CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants