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.

Sign up for GitHub

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

Jump to bottom

feat(css_parser): CSS Parser pseudo element selector #268 #673

Merged
merged 1 commit into from
Nov 7, 2023
Merged

feat(css_parser): CSS Parser pseudo element selector #268 #673

merged 1 commit into from
Nov 7, 2023

Conversation

denbezrukov
Copy link
Contributor

Summary

Better error recovery for missing close tokens:

::highlight(div + div) {}
::highlight( {}
::highlight(

::cue(div + div {}
::cue( {}
::cue(

Test Plan

cargo test

@denbezrukov
feat(css_parser): CSS Parser pseudo element selector #268
269f463 
- better error recovery for missing close tokens
@denbezrukov denbezrukov temporarily deployed to Website deployment November 4, 2023 18:42 — with GitHub Actions Inactive
@github-actions github-actions bot added A-Parser Area: parser A-Tooling Area: internal tools L-CSS Language: CSS labels Nov 4, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2023

Parser conformance results on

js/262

Test result main count This PR count Difference
Total 49701 49701 0
Passed 48721 48721 0
Failed 980 980 0
Panics 0 0 0
Coverage 98.03% 98.03% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6322 6322 0
Passed 2036 2036 0
Failed 4286 4286 0
Panics 0 0 0
Coverage 32.20% 32.20% 0.00%

ts/babel

Test result main count This PR count Difference
Total 662 662 0
Passed 592 592 0
Failed 70 70 0
Panics 0 0 0
Coverage 89.43% 89.43% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17646 17646 0
Passed 13454 13454 0
Failed 4190 4190 0
Panics 2 2 0
Coverage 76.24% 76.24% 0.00%

ematipico
ematipico approved these changes Nov 7, 2023
View reviewed changes
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.

This is a great improvement! I left only a comment, but it's trivial and we can address it later

crates/biome_css_parser/src/syntax/selector.rs
@@ -364,9 +365,15 @@ pub(crate) fn parse_pseudo_element_function(p: &mut CssParser) -> ParsedSyntax {
parse_selector(p).or_add_diagnostic(p, expect_any_selector);

let context = selector_lex_context(p);
p.expect_with_context(T![')'], context);
if !p.eat_with_context(T![')'], context)
&& ParseRecovery::new(CSS_BOGUS, PSEUDO_ELEMENT_FUNCTION_RECOVERY_SET)
Copy link
Member

Choose a reason for hiding this comment

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

Any chance to have a better bogus here? Usually, we try to give specific names based on what we are parsing. This means that eventually CSS_BOGUS will disappear. I initially created it because we didn't have a parser yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, because it could be anything. We may need to skip some nodes while searching for '{'.

@denbezrukov denbezrukov merged commit 9d95987 into main Nov 7, 2023
16 checks passed
@denbezrukov denbezrukov deleted the pseudo-element-recovery branch November 7, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ematipico ematipico ematipico approved these changes

Assignees
No one assigned
Labels
A-Parser Area: parser A-Tooling Area: internal tools L-CSS Language: CSS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@denbezrukov @ematipico