-
-
Notifications
You must be signed in to change notification settings - Fork 511
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(parse/css): change fields in CssParserSettings
to Option
#3273
refactor(parse/css): change fields in CssParserSettings
to Option
#3273
Conversation
CodSpeed Performance ReportMerging #3273 will not alter performanceComparing Summary
|
17cf082
to
bbf793e
Compare
bbf793e
to
9664d4a
Compare
CssParserOptions
and CssParserSettings
to Option
CssParserSettings
to Option
e80407f
to
ca452cc
Compare
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.
It seems we made some mistake somewhere (very difficult to see from the PR). The CSS linter should be disabled by default, so the failed test is legit and we should not update it
This is addressed in a better wayThe problem is here: biome/crates/biome_service/src/settings.rs Lines 248 to 252 in 7935235
It should be changed to: /// Whether the formatter is disabled for CSS files
pub fn css_formatter_disabled(&self) -> bool {
let enabled = self.languages.css.formatter.enabled.as_ref();
// TODO: Change me when CSS formatter is enabled by default.
enabled != Some(&true)
} And the default here doesn't work because biome/crates/biome_service/src/file_handlers/css.rs Lines 53 to 64 in 7935235
I didn't check how and why it worked before. The logic is a little complicated. Maybe we should do the same change for biome/crates/biome_service/src/settings.rs Lines 266 to 270 in 7935235
|
ca452cc
to
a29751d
Compare
Basically, the IMO a better fix would be to |
Ah shoot, I didn't see that coming |
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.
Thanks! I wonder if you plan to refactor the JsParserSettings
?
a29751d
to
433d9e6
Compare
Co-authored-by: Ze-Zheng Wu <[email protected]>
I can open an issue for refactoring Edit: I've opened #3297 for this |
…biomejs#3273) Co-authored-by: Ze-Zheng Wu <[email protected]>
Summary
This changes the CSS parser options to use
Option<bool>
for better semantics when dealing with default values and fallbacks.basically the same change as #3272
Test Plan
Ci should pass