-
-
Notifications
You must be signed in to change notification settings - Fork 670
feat(biome_css_analyze): add rule useSortedProperties
#4063
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_analyze): add rule useSortedProperties
#4063
Conversation
useSortedProperties
useSortedProperties
useSortedProperties
CodSpeed Performance ReportMerging #4063 will degrade performances by 20.34%Comparing Summary
Benchmarks breakdown
|
Thank you @simon-paris for the contribution. I will look into this rule this week. As a very early feedback, this rule should be made an assist (apologies for the lack of documentation, I will try to fix that ASAP). Here's an example of assist: https://github.com/biomejs/biome/blob/main/crates/biome_json_analyze/src/assists/source/use_sorted_keys.rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @simon-paris for the late, very late review.
I finally managed to understand your code and look at it. There's a lot of room for improvement. I suggest to check how use_sorted_keys
is implemented, and emulate the same behaviour/implementation.
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
Thanks for the feedback! I've addressed most of the issues already, I'll finish it up on monday. |
Alrighty everything is addressed, ready for re-review. Main changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second review. Main points:
- Technical documentation is not enough; we should strive to set in stone as much doc as possible
- User documentation is not enough; we should provide more exhaustive examples
- The diagnostic should be slightly improved to match our rule pillars
- The code, generally, can be simplified
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/use_sorted_properties.rs
Outdated
Show resolved
Hide resolved
@ematipico Thanks! I've addressed most of the comments, except the biome_string_case issue and the original_position issue which need clarification. Main changes:
Thank you for your patience 🙏 |
Yeah |
I've made some further changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @simon-paris for your great contribution! I will make sure to move the rule in the right place after the merge :)
Summary
Related: #3 (comment), #3167
This implements CSS declaration and rule sorting from the
stylelint-order
plugin. It sorts properties (and nested rules) in a consistent order, e.g:The property order comes from the most popular preset config:
stylelint-config-recess-order
.This is the equivalent stylelint config:
The sort order is this: (ties stay in the same relative order)
a. Custom properties
b. The "composes" property
c. Properties
e. Nested rules and at-rules
f. Other
a. By the order defined in PROPERTY_ORDER
a. By the order defined in VENDOR_PREFIXES
b. Properties with no vendor prefix
Some possible issues:
stylelint-config-recess-order has an open PR that changes the order a little bit. It'd be best to wait for that to merge and sync the ordering before promoting this rule.stylelint-config-recess-order doesn't list every property. There are 230 (rarely used) properties that have no defined order.Some newly released properties aren't listed. I'd rather not unilaterally decide where they go so I've left them out for now. This rule silences itself if any property exists that isn't in the list. The stylelint version doesn't raise errors for unknown properties but applying any autofix will move them to the end.Test Plan
Unit tests are included.