8000 feat(rome_css_parser): CSS parser #4682 by denbezrukov · Pull Request #4724 · rome/tools · GitHub
[go: up one dir, main page]
Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

feat(rome_css_parser): CSS parser #4682 - #4724

Merged
denbezrukov merged 1 commit into
mainfrom
feat/css-parser-crate
Jul 26, 2023
Merged

feat(rome_css_parser): CSS parser #4682#4724
denbezrukov merged 1 commit into
mainfrom
feat/css-parser-crate

Conversation

@denbezrukov
@denbezrukov denbezrukov commented Jul 24, 2023 •
edited
Loading
Copy link
Copy Markdown
Contributor

Summary

Initial implementation of CssTokenSource and CssParser for the CSS parser.
Added CssParserOptions which has an allow_single_line_comments option. This allows the writing of single line comments in CSS syntax.

Test Plan

cargo test

@netlify
netlify Bot commented Jul 24, 2023 •
edited
Loading
Copy link
Copy Markdown

✅ Deploy Preview for docs-rometools canceled.

Name Link
🔨 Latest commit 190ed1c
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64bedeb1dfda2a0008514efd

@github-actions github-actions Bot added A-Core Area: core L-CSS Language: CSS A-Parser Area: parser A-Tooling Area: our own build, development, and release tooling L-JSON Language: JSON labels Jul 24, 2023
@denbezrukov
denbezrukov force-pushed the feat/css-parser-crate branch from f0fea7f to fb5fb02 Compare July 24, 2023 20:00
@github-actions
github-actions Bot commented Jul 24, 2023
Copy link
Copy Markdown

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 48863 48863 0
Passed 47810 47810 0
Failed 1053 1053 0
Panics 0 0 0
Coverage 97.84% 97.84% 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 6212 6212 0
Passed 1764 1764 0
Failed 4448 4448 0
Panics 0 0 0
Coverage 28.40% 28.40% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 573 573 0
Failed 66 66 0
Panics 0 0 0
Coverage 89.67% 89.67% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 17224 17224 0
Passed 13121 13121 0
Failed 4103 4103 0
Panics 0 0 0
Coverage 76.18% 76.18% 0.00%

@denbezrukov
denbezrukov force-pushed the feat/css-parser-crate branch from fb5fb02 to 190ed1c Compare July 24, 2023 20:27
@denbezrukov denbezrukov changed the title feat(rome_js_parser): CSS lexer #4682 feat(rome_js_parser): CSS parser #4682 Jul 25, 2023
denbezrukov
denbezrukov commented Jul 25, 2023
View reviewed changes
Comment thread crates/rome_css_parser/src/syntax.rs
use rome_css_syntax::CssSyntaxKind::*;
use rome_parser::Parser;

pub(crate) fn parse_root(p: &mut CssParser) {
@denbezrukov denbezrukov Jul 25, 2023
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's the entry point for syntax parsing.

denbezrukov
denbezrukov commented Jul 25, 2023
View reviewed changes
Comment thread crates/rome_css_parser/tests/spec_test.rs
Undefined,
}

pub fn run(test_case: &str, _snapshot_name: &str, test_directory: &str, outcome_str: &str) {
@denbezrukov denbezrukov Jul 25, 2023
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's a copy of the JSON test spec.

I'd like to write tests the same way we use in the JS parser. But I'm unsure what is the best way. I think that we can combine both approaches.

@ematipico ematipico Jul 26, 2023
Copy link
Copy Markdown
Contributor
8000

Choose a reason for hiding this comment

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

Personally, I don't like the test suite of the JS parser:

  • we have to run two commands in order to run the tests
  • we always need to tweak the test generator parser to accommodate new things - like parser options, which is becoming quite difficult

The best way, personally, would be just to write a file to test and then check the snapshot with insta; of course the testing suite will do much more, but that's the gist of it.

denbezrukov
denbezrukov commented Jul 25, 2023
View reviewed changes
Comment thread crates/rome_json_parser/src/parser.rs
}

/// Try to eat a specific token kind, if the kind is not there then adds an error to the events stack.
fn expect(&mut self, kind: Self::Kind) -> bool {
@denbezrukov denbezrukov Jul 25, 2023
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It seems that we don't need any special behavior for comments here. When allow_comments is true, it implies that COMMENT and MULTILINE_COMMENT kinds are considered as trivia.

tools/crates/rome_json_parser/src/token_source.rs

Lines 62 to 71 in 1dcd62b

Ok(trivia_kind) => {
if trivia_kind.is_newline() {
trailing = false;
self.preceding_line_break = true;
}
self.trivia
.push(Trivia::new(trivia_kind, token.range(), trailing));
}
}

@denbezrukov
denbezrukov requested a review from ematipico July 25, 2023 09:01
@denbezrukov denbezrukov changed the title feat(rome_js_parser): CSS parser #4682 feat(rome_css_parser): CSS parser #4682 Jul 26, 2023
@denbezrukov
denbezrukov merged commit a3a13d7 into main Jul 26, 2023
@denbezrukov
denbezrukov deleted the feat/css-parser-crate branch July 26, 2023 20:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@ematipico ematipico ematipico approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Core Area: core A-Parser Area: parser A-Tooling Area: our own build, development, and release tooling L-CSS Language: CSS L-JSON Language: JSON

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@denbezrukov @ematipico
0