-
-
Notifications
You must be signed in to change notification settings - Fork 475
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 noInvalidPositionAtImportRule #2717
feat(biome_css_analyzer): implement noInvalidPositionAtImportRule #2717
Conversation
Merge branch 'main' into feature/noInvalidPositionAtImportRule and `just gen-lint`
CodSpeed Performance ReportMerging #2717 will not alter performanceComparing Summary
|
crates/biome_css_analyze/src/lint/nursery/no_invalid_position_at_import_rule.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/no_invalid_position_at_import_rule.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/no_invalid_position_at_import_rule.rs
Outdated
Show resolved
Hide resolved
crates/biome_css_analyze/src/lint/nursery/no_invalid_position_at_import_rule.rs
Outdated
Show resolved
Hide resolved
.note(markup! { | ||
"Consider moving import position." | ||
}).note(markup! { | ||
"Any @import rules must precede all other valid at-rules and style rules in a stylesheet (ignoring @charset and @layer), or else the @import rule is invalid." |
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.
Can you use <Emphasis></Emphasis>
to highlight @import
?
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.
fix: 7495e81
crates/biome_css_analyze/src/lint/nursery/no_invalid_position_at_import_rule.rs
Outdated
Show resolved
Hide resolved
}, | ||
) | ||
.note(markup! { | ||
"Consider moving import position." |
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.
Can you move this at the end? Reminder to check our rule pillars https://biomejs.dev/linter/#rule-pillars
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.
fix: c6f45ab
continue; | ||
} | ||
|
||
let import_rule = any_css_at_rule.as_css_import_at_rule().cloned(); |
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.
Don't clone here. Clone when you actually need it, which is when you call return
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.
fix: 9ca1793
> 2 │ @import 'foo.css'; | ||
│ ^^^^^^^^^^^^^^^^^ |
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.
Not sure if you noticed, but the diagnostic isn't correct because it doesn't highlight the @
.
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.
Thanks for the review.
Sorry, I didn't know how to do it 🙇♂️
Is there a sample somewhere?
type State = CssImportAtRule; | ||
type Signals = Option<Self::State>; |
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.
I think this can be a bit better. Instead of highlighting only the first incorrect at-rule, we should highlight ALL incorrect at-rules. For example:
a {}
@import "foo";
@import "bar";
@import "foo";
@import "bar";
Here we should flag all import all together.
This means we should return a Vec<Self::State>
. And State
should be a TextRange
, we don't need the whole node.
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, I didn't know how to do it 🙇♂️
RuleDiagnostic
only accepts a single TextRange.
How should I handle vector format state in RuleDiagnostic
?
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 infrastructure does that for you, you don't need to do much more.
You just need to return a vector from the run
function
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 for your advice!
I am trying to fix it locally as below.
We add invalid_import_list
and return it at the end.
But I get a TypeError in the state of RuleDiagnostic::new().
crates/biome_css_analyze/src/lint/nursery/no_invalid_position_at_import_rule.rs
impl Rule for NoInvalidPositionAtImportRule {
type Query = Ast<CssRuleList>;
type State = Vec<TextRange>; // ← update
type Signals = Option<Self::State>;
type Options = ();
fn run(ctx: &RuleContext<Self>) -> Option<Self::State> {
let node = ctx.query();
let mut is_invalid_position = false;
let mut invalid_import_list = Vec::new(); // ← update
for rule in node {
let any_css_at_rule = match rule {
AnyCssRule::CssAtRule(item) => item.rule().ok(),
_ => None,
};
if let Some(any_css_at_rule) = any_css_at_rule {
// Ignore @charset, @layer
if any_css_at_rule.as_css_charset_at_rule().is_some() {
continue;
}
if any_css_at_rule.as_css_layer_at_rule().is_some() {
continue;
}
let import_rule = any_css_at_rule.as_css_import_at_rule();
if let Some(import_rule) = import_rule {
if is_invalid_position {
invalid_import_list.push(import_rule.range()); // ← update
}
} else {
is_invalid_position = true;
}
} else {
is_invalid_position = true;
}
}
if invalid_import_list.len() > 0 {
return Some(invalid_import_list); // ← update
}
None
}
fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
Some(
RuleDiagnostic::new(
rule_category!(),
state, // ← TypeError
markup! {
"This "<Emphasis>"@import"</Emphasis>" is in the wrong position."
},
)
.note(markup! {
"Any "<Emphasis>"@import"</Emphasis>" rules must precede all other valid at-rules and style rules in a stylesheet (ignoring @charset and @layer), or else the "<Emphasis>"@import"</Emphasis>" rule is invalid."
}).note(markup! {
"Consider moving import position."
})
)
}
}
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.
type Signals
must return a Vec<Self::State>
instead of Option<Self::State>
.
Basically, you must always return a vector.
State must stay a TextRange.
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.
…at_import_rule.rs Co-authored-by: Emanuele Stoppa <[email protected]>
…at_import_rule.rs Co-authored-by: Emanuele Stoppa <[email protected]>
…at_import_rule.rs Co-authored-by: Emanuele Stoppa <[email protected]>
…at_import_rule.rs Co-authored-by: Emanuele Stoppa <[email protected]>
…at_import_rule.rs Co-authored-by: Emanuele Stoppa <[email protected]>
pub struct RuleState { | ||
span: TextRange, | ||
} |
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.
If you have struct with one field, the struct is superfluous. Just TextRange as state is enough
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.
fix: f536251
@charset 'utf-8'; | ||
@import 'foo.css'; |
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.
I suppose @charset "utf-8"; @import 'foo.css';
passes as well?
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 for your review.
fix: eb6210b
/* some comment */ | ||
@import 'foo.css'; |
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.
nit
Maybe add a test case for
@charset "utf-8";
/* some comment */
@import 'foo.css';
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.
fix: aa879c1
--- | ||
# Input | ||
```css | ||
@CHARSET 'utf-8'; |
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.
major
This is invalid.
i.e. @charset
string must be double-quoted.
It's not really a at-rule per se.
If you want to be pedantic, Firefox used to support the non-standard @charset utf-8;
almost 20 years ago.
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.
fix: 27b6ab5
--- | ||
# Input | ||
```css | ||
@charset 'utf-8'; |
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.
@charset 'utf-8'; | |
@charset "utf-8"; |
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.
fix: eb6210b
@@ -0,0 +1,3 @@ | |||
@CHARSET 'utf-8'; |
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.
major
This is invalid.
i.e. it must be lowercased
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.
fix: 27b6ab5
Summary
close #2709
Implement no-invalid-position-at-import-rule)
Please note that the following is not yet addressed:
Test Plan
added tests and snapshots