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

📎 Implement unit-no-unknown #2515

Closed
Tracked by #2511
togami2864 opened this issue Apr 18, 2024 · 5 comments · Fixed by #2535
Closed
Tracked by #2511

📎 Implement unit-no-unknown #2515

togami2864 opened this issue Apr 18, 2024 · 5 comments · Fixed by #2535
Assignees
Labels
A-Analyzer Area: analyzer A-Linter Area: linter L-CSS Language: CSS S-Help-wanted Status: you're familiar with the code base and want to help the project

Comments

@togami2864
Copy link
Contributor

togami2864 commented Apr 18, 2024

Implmenet unit-no-unknown

Want to contribute? Lets you know you are interested! We will assign you to the issue to prevent several people to work on the same issue. Don't worry, we can unassign you later if you are no longer interested in the issue! Read our contributing guide and analyzer contributing guide.

@togami2864 togami2864 added S-Help-wanted Status: you're familiar with the code base and want to help the project A-Linter Area: linter L-CSS Language: CSS A-Analyzer Area: analyzer labels Apr 18, 2024
@neoki07
Copy link
Contributor

neoki07 commented Apr 18, 2024

Hello! While I haven't contributed to biome before, I'm interested in tackling this issue.

@togami2864
Copy link
Contributor Author

@neokidev Thanks! Feel free to ask if you have any questions, as the documents aren't organized yet:)

@neoki07
Copy link
Contributor

neoki07 commented Apr 20, 2024

I've implemented the basic rules, but I'm still working out the details and have some questions:

  • Currently, the F unit specified in @font-face's unicode-range is being rejected. It seems that we need to check the parent node. Do you have any suggestions for a solution?
  • Also, dimensions including the unit x are being parsed as CssRegularDimension and accepted. Is this tokenization correct?
  • How closely should we mimic stylelint's unit-no-unknown rule?
    • Should we implement stylelint's options such as ignoreUnits and ignoreFunctions?
    • Should we cover syntaxes like postcss-scss and postcss-less as well? (These were included in stylelint's test cases)

You can find the test code for stylelint here:
https://github.com/stylelint/stylelint/blob/main/lib/rules/unit-no-unknown/__tests__/index.mjs

I've created a draft PR (link) with my current implementation, which you can refer to for more context. Thank you for your help!

@togami2864
Copy link
Contributor Author

Currently, the F unit specified in @font-face's unicode-range is being rejected. It seems that we need to check the parent node. Do you have any suggestions for a solution?

I think this is parser's bug.

Also, dimensions including the unit x are being parsed as CssRegularDimension and accepted. Is this tokenization correct?

I think the parser corrects but it's a edge case of this rule. If it's too difficult to solve, please skip it and file an issue.

Should we implement stylelint's options such as ignoreUnits and ignoreFunctions?

If possible. You can split another PR.

Should we cover syntaxes like postcss-scss and postcss-less as well? (These were included in stylelint's test cases)

You can ignore them right now.

@neoki07
Copy link
Contributor

neoki07 commented Apr 20, 2024

I have created the PR (#2535).

However, implementing the options will require more time to catch up, so I will address this in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Analyzer Area: analyzer A-Linter Area: linter L-CSS Language: CSS S-Help-wanted Status: you're familiar with the code base and want to help the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants