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): implement noDuplicateAtImportRules #2658

Merged
merged 5 commits into from
May 2, 2024

Conversation

DerTimonius
Copy link
Contributor

Summary

This PR implements the noDuplicateAtImportRules (stylelint-rule) to the css-analyzer.
It would show an error with the following examples:

@import "a.css";
@import "a.css";
@import url("a.css");
@import "a.css";

The following code would be valid:

@import "a.css";
@import "b.css";

This also includes a check for imported media queries, so the following code would be valid:

@import url("a.css") tv;
@import url("a.css") projection;

This one was the biggest issue for me, I came up with a combination of HashMap and HashSet to check duplicates, there's probably an easier solution I overlooked.

Test Plan

I suppose the test cases in the valid/invalid files are enough?

Closes #2627

@github-actions github-actions bot added A-Project Area: project L-CSS Language: CSS A-Diagnostic Area: diagnostocis labels Apr 30, 2024
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.

Thank you! It would be better to add test cases ported from the stylelint repo. https://github.com/stylelint/stylelint/blob/main/lib/rules/no-duplicate-at-import-rules/__tests__/index.mjs

pub NoDuplicateAtImportRules {
version: "next",
name: "noDuplicateAtImportRules",
recommended: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
recommended: false,
recommended: true,


fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
let node = ctx.query();
// let mut import_url_list = HashSet::new();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed

Comment on lines 114 to 119
"Duplicate @import rule."
},
)
.note(markup! {
"Looks like you imported the same file twice. Consider removing one to get rid of this error!"
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should follow rule pillars for diagnostics.
https://biomejs.dev/linter/#rule-pillars

AnyCssAtRule::CssImportAtRule(import_rule) => {
let import_url = import_rule.url().ok()?.text().to_lowercase();
if let Some(media_query_set) = import_url_map.get_mut(&import_url) {
// if the current import_rule has no media queries or there are no queries safed in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// if the current import_rule has no media queries or there are no queries safed in the
// if the current import_rule has no media queries or there are no queries saved in the

@DerTimonius
Copy link
Contributor Author

@togami2864 Thanks for the review!
I initially wanted to add more test cases to the invalid.css file, but the rule is triggered at the first error and does not continue looking at the other imports. So my guess is that I have to create a new test file? If that's the case, I'm not sure how I get just test to run said test...

@togami2864
Copy link
Contributor

@DerTimonius You can use just test-lintrule <ruleName> to run specific rule:)

Copy link

codspeed-hq bot commented May 1, 2024

CodSpeed Performance Report

Merging #2658 will not alter performance

Comparing DerTimonius:feat/no-duplicate-at-imports (ea2f9dc) with main (5fda633)

Summary

✅ 88 untouched benchmarks

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.

I left small nits. Could you fix and update snapshots?

},
)
.note(markup! {
"To fix this issue, remove one of the duplicated imports."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"To fix this issue, remove one of the duplicated imports."
"Consider removing one of the duplicated imports."

rule_category!(),
span,
markup! {
"Duplicate @import rule."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Duplicate @import rule."
"Each "<Emphasis>"@import"</Emphasis>" should be unique unless differing by media queries."

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.

Thank you!

@togami2864 togami2864 merged commit 150dd0e into biomejs:main May 2, 2024
12 checks passed
@DerTimonius DerTimonius deleted the feat/no-duplicate-at-imports branch May 2, 2024 05:41
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 no-duplicate-at-import-rules
2 participants