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): noDuplicateCustomProperties #2783

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

chansuke
Copy link
Contributor

@chansuke chansuke commented May 9, 2024

Summary

Fixes #2784

Test Plan

@github-actions github-actions bot added A-Project Area: project L-CSS Language: CSS A-Diagnostic Area: diagnostocis labels May 9, 2024
Copy link

codspeed-hq bot commented May 9, 2024

CodSpeed Performance Report

Merging #2783 will not alter performance

Comparing chansuke:feat/declaration-block-no-duplicate-custom-properties (eebb582) with main (83a5c01)

Summary

✅ 84 untouched benchmarks

@chansuke chansuke force-pushed the feat/declaration-block-no-duplicate-custom-properties branch from 8cb3658 to eebb582 Compare May 9, 2024 09:41
Comment on lines 48 to 60
let mut custom_properties: Vec<SyntaxToken<CssLanguage>> = vec![];
for item in node {
if let Some(css_declaration) = item.as_css_declaration_with_semicolon() {
if let Ok(property) = css_declaration.declaration().ok()?.property() {
if let Some(property) = property.as_css_generic_property() {
if let Some(ident) = property.name().ok()?.as_css_identifier() {
let value_token = ident.value_token().ok()?;
custom_properties.push(value_token);
}
}
}
}
}
Copy link
Contributor

@abidjappie abidjappie May 9, 2024

Choose a reason for hiding this comment

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

Opinion: I think you can get rid of the mut here by using a filter_map and it will return an Iterator that you can possible use directly with check_duplicate_custom_properties.

Suggested change
let mut custom_properties: Vec<SyntaxToken<CssLanguage>> = vec![];
for item in node {
if let Some(css_declaration) = item.as_css_declaration_with_semicolon() {
if let Ok(property) = css_declaration.declaration().ok()?.property() {
if let Some(property) = property.as_css_generic_property() {
if let Some(ident) = property.name().ok()?.as_css_identifier() {
let value_token = ident.value_token().ok()?;
custom_properties.push(value_token);
}
}
}
}
}
let custom_properties = node.into_iter().filter_map(|item| {
item.as_css_declaration_with_semicolon()
.and_then(|css_declaration| {
css_declaration
.declaration()
.ok()?
.property()
.ok()?
.as_css_generic_property()
.and_then(|css_generic_property| {
css_generic_property
.name()
.ok()?
.as_css_identifier()
.and_then(|css_identifier| css_identifier.value_token().ok())
})
})
});

You don't need to use it like above, as long as you are returning an Option (Some/None) it should be okay!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abidjappie
I appreciate so much for your feedback!! That's clean and readable.

@ematipico
Copy link
Member

ematipico commented May 11, 2024

I wonder if we should start writing a semantic model.

I think this rule would benefit from it

@chansuke
Copy link
Contributor Author

@ematipico
Thank you for your feedback!
Would it be possible for me to contribute to the creation of a semantic model for CSS?

@ematipico
Copy link
Member

@ematipico Thank you for your feedback! Would it be possible for me to contribute to the creation of a semantic model for CSS?

Sure, I suggest talking with @togami2864, I know he has plans about it

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 declaration-block-no-duplicate-properties
3 participants