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

feat(css_parser):simple block #976

Merged
merged 20 commits into from
Dec 5, 2023
Merged

Conversation

suxin2017
Copy link
Contributor

@suxin2017 suxin2017 commented Nov 30, 2023

#268

Summary

Supports simple css block implementation
support simple func
support declaration list
Support the content

! // is not supported

CssDeclaration =
name: CssIdentifier | CssCustomProperty
':'
value: AnyCssValue
important: CssDeclarationImportant?

AnyCssValue =
CssIdentifier
| CssString
| CssNumber
| CssDimension
| CssRatio
| CssAnyFunction
| CssCustomProperty

Test Plan

cargo test -p biome_css_parser -- --test spec_test::quick_test --ignored

@github-actions github-actions bot added A-Parser Area: parser L-CSS Language: CSS labels Nov 30, 2023
@suxin2017 suxin2017 changed the title Feat css block feat(css_parser) simple block Nov 30, 2023
@suxin2017 suxin2017 changed the title feat(css_parser) simple block feat(css_parser):simple block Nov 30, 2023
@suxin2017 suxin2017 marked this pull request as draft November 30, 2023 11:22
@suxin2017 suxin2017 marked this pull request as ready for review December 1, 2023 02:16
@suxin2017 suxin2017 marked this pull request as draft December 1, 2023 02:18
@suxin2017 suxin2017 marked this pull request as ready for review December 1, 2023 02:34
@github-actions github-actions bot added the A-Tooling Area: internal tools label Dec 1, 2023
@github-actions github-actions bot added the L-JavaScript Language: JavaScript and super languages label Dec 1, 2023
Copy link
Contributor

@denbezrukov denbezrukov left a comment

Choose a reason for hiding this comment

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

Great start with the CSS block! I have some questions and proposals to discuss.

crates/biome_css_parser/src/syntax/mod.rs Outdated Show resolved Hide resolved
crates/biome_css_parser/src/syntax/mod.rs Outdated Show resolved Hide resolved
crates/biome_css_parser/src/syntax/mod.rs Outdated Show resolved Hide resolved
crates/biome_css_parser/src/syntax/mod.rs Outdated Show resolved Hide resolved
crates/biome_css_parser/src/syntax/mod.rs Outdated Show resolved Hide resolved
crates/biome_css_parser/src/syntax/mod.rs Outdated Show resolved Hide resolved
crates/biome_css_parser/src/syntax/mod.rs Outdated Show resolved Hide resolved
crates/biome_css_parser/src/syntax/mod.rs Outdated Show resolved Hide resolved
crates/biome_css_parser/src/lexer/mod.rs Show resolved Hide resolved
Copy link

netlify bot commented Dec 3, 2023

Deploy Preview for rad-torte-839a59 ready!

Name Link
🔨 Latest commit 1544210
🔍 Latest deploy log https://app.netlify.com/sites/rad-torte-839a59/deploys/656e95408efa3f00088acc05
😎 Deploy Preview https://deploy-preview-976--rad-torte-839a59.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@suxin2017
Copy link
Contributor Author

Hello, I have finished the review of the todo, can you help me to review again @denbezrukov

Copy link
Contributor

@denbezrukov denbezrukov left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! We need to address several issues, but overall, it looks good and is a valuable addition to the project.


#[inline]
fn is_unit_str(unit: &str) -> bool {
is_length_unit(unit)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we need to have "em", "rem" as keywords.
See


name: ('matches' | 'not' | 'is' | 'where')

b"media" => MEDIA_KW,
b"keyframes" => KEYFRAMES_KW,
b"and" => AND_KW,
b"only" => ONLY_KW,
b"or" => OR_KW,
b"i" => I_KW,
b"s" => S_KW,
b"important" => IMPORTANT_KW,
b"from" => FROM_KW,
b"to" => TO_KW,
b"var" => VAR_KW,
b"highlight" => HIGHLIGHT_KW,
b"part" => PART_KW,
b"has" => HAS_KW,
b"dir" => DIR_KW,
b"global" => GLOBAL_KW,
b"local" => LOCAL_KW,
b"-moz-any" => _MOZ_ANY_KW,
b"-webkit-any" => _WEBKIT_ANY_KW,
b"past" => PAST_KW,
b"current" => CURRENT_KW,
b"future" => FUTURE_KW,
b"host" => HOST_KW,
b"host-context" => HOST_CONTEXT_KW,
b"not" => NOT_KW,
b"matches" => MATCHES_KW,
b"is" => IS_KW,
b"where" => WHERE_KW,
b"lang" => LANG_KW,
b"of" => OF_KW,
b"n" => N_KW,
b"even" => EVEN_KW,
b"odd" => ODD_KW,
b"nth-child" => NTH_CHILD_KW,
b"nth-last-child" => NTH_LAST_CHILD_KW,
b"nth-of-type" => NTH_OF_TYPE_KW,
b"nth-last-of-type" => NTH_LAST_OF_TYPE_KW,
b"nth-col" => NTH_COL_KW,
b"nth-last-col" => NTH_LAST_COL_KW,
b"ltr" => LTR_KW,
b"rtl" => RTL_KW,
b"charset" => CHARSET_KW,
b"color-profile" => COLOR_PROFILE_KW,
b"counter-style" => COUNTER_STYLE_KW,

We can move them to keywords in another PR.

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 tried but got the following error, we can fix it in the next pr.

-> biome\crates\biome_parser\src\token_set.rs:46:18
   |
46 |         _ => [0, 1u128 << (num - 127)],
   |                  ^^^^^^^^^^^^^^^^^^^^ attempt to shift left by `170_usize`, which would overflow

We may need a modification such as the following

--- a/crates/biome_parser/src/token_set.rs
+++ b/crates/biome_parser/src/token_set.rs
@@ -2,10 +2,10 @@ use biome_rowan::SyntaxKind;
 use std::marker::PhantomData;

 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub struct TokenSet<K: SyntaxKind>([u128; 2], PhantomData<K>);
+pub struct TokenSet<K: SyntaxKind>([u128; 3], PhantomData<K>);

 impl<K: SyntaxKind> TokenSet<K> {
-    pub const EMPTY: TokenSet<K> = TokenSet([0; 2], PhantomData);
+    pub const EMPTY: TokenSet<K> = TokenSet([0; 3], PhantomData);

     pub fn singleton(kind: K) -> Self {
         unsafe { TokenSet::from_raw(kind.to_raw().0) }
@@ -13,7 +13,7 @@ impl<K: SyntaxKind> TokenSet<K> {

     pub const fn union(self, other: TokenSet<K>) -> Self {
         TokenSet(
-            [self.0[0] | other.0[0], self.0[1] | other.0[1]],
+            [self.0[0] | other.0[0], self.0[1] | other.0[1],self.0[2] | other.0[2]],
             PhantomData,
         )
     }
@@ -23,7 +23,8 @@ impl<K: SyntaxKind> TokenSet<K> {
         let num = kind as usize;
         match num {
             0..=127 => self.0[0] & mask(kind)[0] != 0,
-            _ => self.0[1] & mask(kind)[1] != 0,
+            128..=255 => self.0[1] & mask(kind)[1] != 0,
+            _ => self.0[2] & mask(kind)[2] != 0,
         }
     }

@@ -39,11 +40,12 @@ impl<K: SyntaxKind> TokenSet<K> {
     }
 }

-const fn mask(kind: u16) -> [u128; 2] {
+const fn mask(kind: u16) -> [u128; 3] {
     let num = kind as usize;
     match num {
-        0..=127 => [1u128 << num, 0],
-        _ => [0, 1u128 << (num - 127)],
+        0..=127 => [1u128 << num, 0,0],
+        128..=255 => [0,1u128 << (num - 127), 0],
+        _ => [0,0, 1u128 << (num - 255)],
     }
 }

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see..
Maybe we can remove all colours from keywords for now.
Anyway we can do it in another PR.

crates/biome_css_parser/src/syntax/mod.rs Outdated Show resolved Hide resolved
crates/biome_css_parser/src/syntax/css_dimension.rs Outdated Show resolved Hide resolved
use super::{parse_regular_identifier, parse_regular_number};

#[inline]
pub(crate) fn is_at_css_dimension(p: &mut CssParser) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that it's better to remove css prefix. What do you think?
I believe that we need the prefix only for public functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@denbezrukov
Copy link
Contributor

@suxin2017 Thank you sincerely for your valuable contribution.

@denbezrukov denbezrukov merged commit 6e65006 into biomejs:main Dec 5, 2023
16 checks passed
@suxin2017
Copy link
Contributor Author

@suxin2017 Thank you sincerely for your valuable contribution.

you are welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Parser Area: parser A-Tooling Area: internal tools L-CSS Language: CSS L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants