feat: add no-invalid-at-rule-placement rule#171
Conversation
lumirlumir
added
the
accepted
jeddy3
left a comment
jeddy3
left a comment
There was a problem hiding this comment.
Nice and useful rule that plugs a gap, as CSSTree only detected misplaced declarations, e.g.:
--foo: bar; /* CSSTree parse error */
a { }
nzakas
left a comment
nzakas
left a comment
There was a problem hiding this comment.
LGTM. Would like another review before merging.
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
LGTM! I don't think any of my comments are blocking - they can all be either done as followups or wontfixed IMO.
I would like to see the responses to snitin315's comments before merge though.
| At-rules are CSS statements that instruct CSS how to behave. Some at-rules have strict placement requirements that must be followed for the stylesheet to work correctly. For example: | ||
|
|
||
| - The `@charset` rule must be placed at the very beginning of a stylesheet, before any other rules, comments, or whitespace. | ||
| - The `@import` rule must be placed at the beginning of a stylesheet, before any other at-rules (except `@charset` and `@layer` statements) and style rules. |
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/CSS/@namespace#description - @namespace also needs to be validated. Should we include that in this initial version of the rule as well?
There was a problem hiding this comment.
I think yes, we should add support for @namespace too, since this is a general rule, and the effort should not be high.
There was a problem hiding this comment.
Agreed—supporting @namespace makes sense, but since this PR is ready, let’s address it in a separate change. Its logic would need to enforce placement after @import but before other rules, which introduces non-trivial complexity.
There was a problem hiding this comment.
@thecalamiity I’d be happy to draft a PR for the @namespace validation
There was a problem hiding this comment.
Since we're already aware of the @namespace case and this rule is intended to cover all at-rules, I believe it makes sense to implement it in the same PR.
That said, I'd love to hear the team's input. @eslint/eslint-team do you think we should include it in this PR or address it in a follow-up?
There was a problem hiding this comment.
Since this PR is ready and there's already a draft PR to add @namespace, I'd say we move forward with this PR as-is.
There was a problem hiding this comment.
Cool, let's merge this then.
@TKDev7 Thanks for the draft. For future contributions, please create an issue or wait for team alignment before starting work on significant changes. This ensures everyone is on the same page and the effort aligns with the team's direction.
thecalamiity
dismissed stale reviews from JoshuaKGoldberg and nzakas
via
a0a73a0
|
@thecalamiity There are few merge conflicts, can you please resolve? |
|
@snitin315 this is ready for merge once you've verified everything. |
github-project-automation
Bot
moved this from Second Review Needed
to Complete
in Triage
8 participants
Prerequisites checklist
What is the purpose of this pull request?
This pull request introduces a new rule no-invalid-at-rule-placement that enforces correct placement of at-rules within stylesheets.
What changes did you make? (Give an overview)
Implemented the no-invalid-at-rule-placement rule, along with documentation and tests.
Related Issues
Fixes #163
Fixes #151
Is there anything you'd like reviewers to focus on?