Skip to content

feat: no-unknown-at-rules -> no-invalid-at-rules#12

Merged
nzakas merged 10 commits into
mainfrom
no-invalid-at-rules
Nov 26, 2024
Merged

feat: no-unknown-at-rules -> no-invalid-at-rules#12
nzakas merged 10 commits into
mainfrom
no-invalid-at-rules

Conversation

@nzakas

@nzakas nzakas commented Nov 19, 2024

Copy link
Copy Markdown
Member

Prerequisites checklist

What is the purpose of this pull request?

Renamed no-unknown-at-rules to no-invalid-at-rules to cover more cases.

What changes did you make? (Give an overview)

  • Renamed source files from no-unknown-at-rules.* to no-invalid-at-rules.*
  • Added checks for invalid preludes, unknown descriptors, and invalid descriptor values.
  • Added more tests
  • Updated docs

Related Issues

Is there anything you'd like reviewers to focus on?

There are type errors that will be resolved once #11 is merged and I can rebase on top of it.

@jeddy3

jeddy3 commented Nov 19, 2024

Copy link
Copy Markdown

Looks ace!

Can I ask if the ambition is to integrate more specific validity cases (not caught by csstree) into this rule or add separate rules for those?

For example, @property requires some descriptors:

@property rules require a syntax and inherits descriptor; if either are missing, the entire rule is invalid and must be ignored. The initial-value descriptor is optional only if the syntax is the universal syntax definition, otherwise the descriptor is required; if it’s missing, the entire rule is invalid and must be ignored.

It'd catch an easy mistake for people to make.

@nzakas

nzakas commented Nov 19, 2024

Copy link
Copy Markdown
Member Author

@jeddy3

Can I ask if the ambition is to integrate more specific validity cases (not caught by csstree) into this rule or add separate rules for those?

I honestly haven't thought that far ahead yet. 😄 I could imagine wanting that check in this rule, but I'm not 100% sure yet. I'd like to just start with what csstree is providing for the first version and then think about things a bit more once we've got some feedback.

@mdjermanovic

Copy link
Copy Markdown
Member

There are merge conflicts now. Also, some tests were failing.

@nzakas nzakas force-pushed the no-invalid-at-rules branch from ad53575 to 7128750 Compare November 20, 2024 16:48
@nzakas nzakas force-pushed the no-invalid-at-rules branch from 3612be5 to 213f3f9 Compare November 22, 2024 16:12
Comment thread src/util.js Outdated
mdjermanovic
mdjermanovic previously approved these changes Nov 23, 2024

@mdjermanovic mdjermanovic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!

I'm only not sure if we would want to remove unused function isSyntaxReferenceError, and if this really needs to be merged with the feat! tag since the first version of this package has not been released yet and a similar kind of change #11 was merged with just the feat tag.

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Nov 23, 2024

@mdjermanovic mdjermanovic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks! I'd merge this but still not sure if we want to use the feat! tag or maybe just feat.

@jeddy3

jeddy3 commented Nov 26, 2024

Copy link
Copy Markdown

Just a quick +1 for the name change. We have an inconsistent mix of no-unknown and no-valid rules in Stylelint, and in hindsight, we should have consistently used no-invalid for spec-compliance things and no-unknown (or something like no-undefined) for user-defined things like animations and custom properties.

@nzakas nzakas changed the title feat!: no-unknown-at-rules -> no-invalid-at-rules feat: no-unknown-at-rules -> no-invalid-at-rules Nov 26, 2024
@nzakas

nzakas commented Nov 26, 2024

Copy link
Copy Markdown
Member Author

@mdjermanovic good point! Because this package hasn't been released yet, "feat" makes more sense.

@nzakas nzakas merged commit b90ee0e into main Nov 26, 2024
@nzakas nzakas deleted the no-invalid-at-rules branch November 26, 2024 15:31
@github-actions github-actions Bot mentioned this pull request Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

accepted There is consensus among the team that this change meets the criteria for inclusion breaking feature

Projects

Status: Complete

Development

Successfully merging this pull request may close these issues.

3 participants