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(parse/css): change fields in CssParserSettings to Option #3273

Conversation

dyc3
Copy link
Contributor

@dyc3 dyc3 commented Jun 24, 2024

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

@github-actions github-actions bot added A-Project Area: project A-Parser Area: parser L-CSS Language: CSS labels Jun 24, 2024
Copy link

codspeed-hq bot commented Jun 24, 2024

CodSpeed Performance Report

Merging #3273 will not alter performance

Comparing dyc3:06-23-refactor_parse_css_change_fields_in_cssparseroptions_and_cssparsersettings_to_option_ (101a3e4) with main (7935235)

Summary

✅ 90 untouched benchmarks

@dyc3 dyc3 force-pushed the 06-23-refactor_parse_css_change_fields_in_cssparseroptions_and_cssparsersettings_to_option_ branch from 17cf082 to bbf793e Compare June 24, 2024 12:16
@dyc3 dyc3 marked this pull request as draft June 24, 2024 12:52
@dyc3 dyc3 force-pushed the 06-23-refactor_parse_css_change_fields_in_cssparseroptions_and_cssparsersettings_to_option_ branch from bbf793e to 9664d4a Compare June 24, 2024 13:13
@dyc3 dyc3 changed the title refactor(parse/css): change fields in CssParserOptions and CssParserSettings to Option refactor(parse/css): change fields in CssParserSettings to Option Jun 25, 2024
@dyc3 dyc3 force-pushed the 06-23-refactor_parse_css_change_fields_in_cssparseroptions_and_cssparsersettings_to_option_ branch 2 times, most recently from e80407f to ca452cc Compare June 26, 2024 13:23
@dyc3 dyc3 marked this pull request as ready for review June 26, 2024 13:24
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.

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

@Sec-ant
Copy link
Member

Sec-ant commented Jun 26, 2024

This is addressed in a better way

The problem is here:

/// Whether the formatter is disabled for CSS files
pub fn css_formatter_disabled(&self) -> bool {
let enabled = self.languages.css.formatter.enabled.as_ref();
enabled == Some(&false)
}

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 CssFormatterSettings is not None when running the biome format command, it's a Some with all the fields set to None:

impl Default for CssFormatterSettings {
fn default() -> Self {
Self {
enabled: Some(false),
indent_style: Default::default(),
indent_width: Default::default(),
line_ending: Default::default(),
line_width: Default::default(),
quote_style: Default::default(),
}
}
}

I didn't check how and why it worked before. The logic is a little complicated.


Maybe we should do the same change for css_linter_disabled to disable the linter by default when running biome lint command.

/// Whether the linter is disabled for CSS files
pub fn css_linter_disabled(&self) -> bool {
let enabled = self.languages.css.linter.enabled.as_ref();
enabled == Some(&false)
}

@dyc3 dyc3 force-pushed the 06-23-refactor_parse_css_change_fields_in_cssparseroptions_and_cssparsersettings_to_option_ branch from ca452cc to a29751d Compare June 26, 2024 15:05
@dyc3
Copy link
Contributor Author

dyc3 commented Jun 26, 2024

Basically, the Default impl for PartialCssFormatter has enabled: None, which makes it fall back to the default for formatting config, which is enabled: true.

IMO a better fix would be to impl Default for PartialCssFormatter, but it appears the Partial macro implements it automatically.

@dyc3 dyc3 requested a review from ematipico June 26, 2024 15:12
@ematipico
Copy link
Member

Ah shoot, I didn't see that coming

@ematipico ematipico requested review from a team and removed request for a team June 26, 2024 15:23
Copy link
Member

@Sec-ant Sec-ant left a 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?

crates/biome_service/src/settings.rs Show resolved Hide resolved
crates/biome_service/src/settings.rs Show resolved Hide resolved
@dyc3 dyc3 force-pushed the 06-23-refactor_parse_css_change_fields_in_cssparseroptions_and_cssparsersettings_to_option_ branch from a29751d to 433d9e6 Compare June 26, 2024 16:17
@dyc3 dyc3 requested a review from Sec-ant June 26, 2024 16:17
crates/biome_service/src/settings.rs Outdated Show resolved Hide resolved
crates/biome_service/src/settings.rs Outdated Show resolved Hide resolved
Co-authored-by: Ze-Zheng Wu <zezhengwu@proton.me>
@dyc3
Copy link
Contributor Author

dyc3 commented Jun 26, 2024

I can open an issue for refactoring JsParserSettings, but I want to focus on editorconfig support so I don't think I'll get to it right away.

Edit: I've opened #3297 for this

@Sec-ant Sec-ant merged commit 91eca89 into biomejs:main Jun 27, 2024
12 checks passed
@dyc3 dyc3 deleted the 06-23-refactor_parse_css_change_fields_in_cssparseroptions_and_cssparsersettings_to_option_ branch June 27, 2024 11:03
rishabh3112 pushed a commit to rishabh3112/biome that referenced this pull request Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Parser Area: parser A-Project Area: project L-CSS Language: CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants