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: no-unknown-at-rules -> no-invalid-at-rules #12

Merged
merged 10 commits into from
Nov 26, 2024
Merged

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Nov 19, 2024

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
Copy link

jeddy3 commented Nov 19, 2024

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
Copy link
Member Author

nzakas commented Nov 19, 2024

@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
Member

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

src/util.js Outdated Show resolved Hide resolved
mdjermanovic
mdjermanovic previously approved these changes Nov 23, 2024
Copy link
Member

@mdjermanovic mdjermanovic left a comment

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
Copy link
Member

@mdjermanovic mdjermanovic left a comment

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
Copy link

jeddy3 commented Nov 26, 2024

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
Copy link
Member Author

nzakas commented Nov 26, 2024

@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
15 checks passed
@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