-
-
Notifications
You must be signed in to change notification settings - Fork 663
feat(linter): implement noDuplicateProperties
#4029
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
Conversation
CodSpeed Performance ReportMerging #4029 will degrade performances by 6.44%Comparing Summary
Benchmarks breakdown
|
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.
Lets try to address the perf regression a little.
crates/biome_css_analyze/src/lint/nursery/no_duplicate_properties.rs
Outdated
Show resolved
Hide resolved
|
||
for declaration in rule.declarations.iter() { | ||
let property = &declaration.property; | ||
let prop_name = property.name.to_lowercase(); |
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.
You could add and use an alternative to to_ascii_lowercase_cow
in biome_string_case
eg to_lowercase_cow
I think allmost part of this perf regression is caused by the use of the CSS Semantic Model. I'll see if there's room for performance improvement. |
We should really have a look at that, and understand if the cause is the semantic model or the rule. You could create a "fake" rule that uses the semantic model, and always trigger the rule. I believe it's important, because we don't have these kinds of regressions in the JavaScript rules, and when we do, it's usually caused by the business logic of the rule. |
markup! { | ||
"Duplicate properties are not allowed." | ||
}, | ||
) | ||
.note(markup! { | ||
"Consider removing the duplicate property." | ||
}), |
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.
Let's remember the rule pillars: https://biomejs.dev/linter/#rule-pillars
This rule doesn't explain why duplicated properties aren't allowed.
> 8 │ color: pink; | ||
│ | ||
> 9 │ color: pink; |
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.
The text range seems incorrect, did you notice the >
character is on line 8? It should not.
Also, It would be nice to add a new note that shows where the first instance of the property is.
Summary
closes #2784
Test Plan
Added tests and snapshots