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(biome_css_analyzer): noUnknownUnit #2535

Merged
merged 26 commits into from
Apr 23, 2024

Conversation

neoki07
Copy link
Contributor

@neoki07 neoki07 commented Apr 20, 2024

Summary

close #2515
Implement unit-no-unknown

However, the stylelint options ignoreUnits and ignoreFunctions are not implemented.

Test Plan

added tests and snapshots

@github-actions github-actions bot added A-Project Area: project L-CSS Language: CSS A-Diagnostic Area: diagnostocis labels Apr 20, 2024
@neoki07 neoki07 marked this pull request as ready for review April 20, 2024 18:01
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 have brought stylelint tests (link). However, root { --margin: 10px + 10pix; } was not added because the syntax seems incorrect.

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 have brought stylelint tests (link). However, @font-face { unicode-range: U+0100-024F; } was not added because this F is parsed as an unknown unit.

@neoki07 neoki07 marked this pull request as draft April 20, 2024 18:10
@neoki07 neoki07 marked this pull request as ready for review April 20, 2024 18:18
Copy link

codspeed-hq bot commented Apr 21, 2024

CodSpeed Performance Report

Merging #2535 will improve performances by 34.29%

Comparing neokidev:feat/unit-no-unknown (1f077ac) with main (a094dac)

Summary

⚡ 1 improvements
✅ 92 untouched benchmarks

Benchmarks breakdown

Benchmark main neokidev:feat/unit-no-unknown Change
big5-added.json[cached] 2.9 ms 2.1 ms +34.29%

@neoki07
Copy link
Contributor Author

neoki07 commented Apr 21, 2024

Sorry. I wasn't sure how to add elements to RECOMMENDED_RULES and RECOMMENDED_RULES_AS_FILTERS in crates/biome_configuration/src/linter/rules.rs and edited them directly. I have now realized that the correct approach is to set recommended: true within the declare_rule! macro. I have committed this change accordingly.

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.

I don't think we need the options for the time being. We can evaluate some options if some users actually need them.

const RESOLUTION_MEDIA_FEATURE_NAMES: [&str; 3] =
["resolution", "min-resolution", "max-resolution"];

fn is_css_hack_unit(value: &str) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please document what this hack is for and why it exists?

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 have added a description of the function and the reason for using this function where it's called.

crates/biome_css_analyze/src/utils.rs Outdated Show resolved Hide resolved
@neoki07
Copy link
Contributor Author

neoki07 commented Apr 21, 2024

I don't think we need the options for the time being. We can evaluate some options if some users actually need them.

Understood. I will not add the options for now. (Implementing these options, which in stylelint accept a list of strings or regular expressions can be complex. I feel it would be prudent to carefully decide on the specifications when they are implemented.)

@neoki07 neoki07 requested a review from ematipico April 21, 2024 09:26
@neoki07 neoki07 requested a review from ematipico April 21, 2024 22:32
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.

Thank you!

Copy link
Contributor

@togami2864 togami2864 left a comment

Choose a reason for hiding this comment

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

Awesome!

@togami2864 togami2864 merged commit 7ea5dff into biomejs:main Apr 23, 2024
12 checks passed
@Mouvedia
Copy link

ignoreUnits and ignoreFunctions are not implemented

Can you add a table for all the rules that are being implemented?
e.g. support: full / partial / × or green / yellow / red

@neoki07
Copy link
Contributor Author

neoki07 commented Apr 26, 2024

Can you add a table for all the rules that are being implemented?

The only options implemented in stylelint are ignoreUnits and ignoreFunctions.

Therefore, the table will look as follows:

Option Status
ignoreUnits ×
ignoreFunctions ×

@Mouvedia
Copy link

@neokidev Sorry I meant a centralised table for all rules so that users can easily tell what's the implementation status of the rules. The details as to why the rules are partially implemented can be added later on.

@ematipico
Copy link
Member

@Mouvedia we don't have a place for that, with so much granularity. The best we can provide is:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Diagnostic Area: diagnostocis A-Project Area: project L-CSS Language: CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📎 Implement unit-no-unknown
4 participants