-
-
Notifications
You must be signed in to change notification settings - Fork 503
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
fix(biome_css_formatter): update css format style #4476
base: main
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #4476 will degrade performances by 10.07%Comparing Summary
Benchmarks breakdown
|
Thank you @eryue0220 for the contribution. Did you take inspiration from Prettier to apply the fix? If so, can you point us to the original code? Also, there's an evident regression in performance, which isn't good, usually. We will help you with that, however it would be great if you could at least explain to us the logic of your algorithm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should look after the regressions. Once we have a clear idea of what this PR does, we can help with that. For now, I left some preliminary feedback
@@ -1,5 +1,6 @@ | |||
--- | |||
source: crates/biome_formatter_test/src/snapshot_builder.rs | |||
assertion_line: 212 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you see this line,it means you're using an old version of cargo-insta
. Run just upgrade-tools
to update the tools we use internally.
@@ -12,6 +12,45 @@ impl FormatNodeRule<CssGenericProperty> for FormatCssGenericProperty { | |||
value, | |||
} = node.as_fields(); | |||
|
|||
let is_dash_identity = name.clone().unwrap().as_css_dashed_identifier().is_some(); | |||
|
|||
let has_comma = value.iter().any(|v| v.text().eq(",")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking for a comma (,
) like this isn't correct. You should rely on the CST/AST, so you don't need to transform a node into a test.
You can use the playground to surgically understand how to query it: https://biomejs.dev/playground/?lintRules=all&files.main.css=aAB0AG0AbAAgAHsACgAJAGYAbwBuAHQALQBmAGEAbQBpAGwAeQA6ACAAcwB5AHMAdABlAG0ALQB1AGkALAAgAC0AYQBwAHAAbABlAC0AcwB5AHMAdABlAG0AOwAKAH0A
(Click on the comma on the left side, and it should point to the relative node on the right, in the syntax tab)
@@ -12,6 +12,45 @@ impl FormatNodeRule<CssGenericProperty> for FormatCssGenericProperty { | |||
value, | |||
} = node.as_fields(); | |||
|
|||
let is_dash_identity = name.clone().unwrap().as_css_dashed_identifier().is_some(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use unwrap()
; you rarely need to use this method because it panics at runtime. In your case you can use CssDashedIdentifer::can_cast(name.syntax().kind())
;
name.format(), | ||
colon_token.format(), | ||
indent(&format_with(|f| { | ||
let last = value.last().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No unwrap
for (idx, v) in value.iter().enumerate() { | ||
if idx == 0 { | ||
write!(f, [soft_line_break_or_space()])?; | ||
} | ||
|
||
if v.text().eq(",") { | ||
write!(f, [v.format(), hard_line_break()])?; | ||
} else if let Some(next) = value.iter().nth(idx + 1) { | ||
if next.text().eq(",") || v == last { | ||
write!(f, [v.format()])?; | ||
} else { | ||
write!(f, [v.format(), space()])?; | ||
} | ||
} else { | ||
write!(f, [v.format()])?; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really sure I understand what's happening here, so I am waiting for your to update the PR description and explain how the algorithm works.
@@ -1,5 +1,6 @@ | |||
--- | |||
source: crates/biome_formatter_test/src/snapshot_builder.rs | |||
assertion_line: 212 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update cargo-insta
and revert these changes
Currently, I just implemented this by my own. The main idea is just to check if every css property value includes comma or not, if true then just inserts a line_break. And the exception is the dash property key, then it should break according to the max-width, which is following the prettier result. I agreed, I'll check the prettier implementation and try to fix the performance problem. |
…into fix/css-formater-update * 'fix/css-formater-update' of github.com:eryue0220/biome:
Summary
Close #4328
Close #4298
Test Plan