Skip to content

Conversation

@nzakas
Copy link
Member

@nzakas nzakas commented Nov 18, 2024

Prerequisites checklist

What is the purpose of this pull request?

Create a new no-invalid-properties rule.

What changes did you make? (Give an overview)

  • Moved logic from no-unknown-properties into no-invalid-properties
  • Moved logic from no-invalid-property-values into no-invalid-properties
  • Added tests for no-invalid-properties
  • Added docs for no-invalid-properties
  • Removed old code for no-unknown-properties and no-invalid-property-values

Related Issues

Refs #9

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

@eslint-github-bot
Copy link

Hi @nzakas!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

  • The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
  • There should be a space following the initial tag and colon, for example 'feat: Message'.
  • The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

@nzakas nzakas mentioned this pull request Nov 18, 2024
1 task
@nzakas nzakas changed the title No valid properties combined feat: no-invalid-properties rule Nov 18, 2024
@jeddy3
Copy link

jeddy3 commented Nov 18, 2024

Merging the rules looks good. It avoids that unnecessary check and means you can choose to add syntax customisation at either the rule or language level.

I think the rule currently checks both property declarations and descriptor declarations rather than just property ones.

You may want to add valid tests for:

  • @font-face { font-weight: 100 400 }
  • @property --foo { syntax: "*"; inherits: false; }

As I think it'll incorrectly flag:

  • 100 400 as an invalid value because the font-weight property accepts <font-weight-absolute> | bolder | lighter whereas the font-weight descriptor accepts <font-weight-absolute>{1,2}
  • syntax and inherits as (unknown) properties because they are descriptors

@nzakas
Copy link
Member Author

nzakas commented Nov 18, 2024

I think the rule currently checks both property declarations and descriptor declarations rather than just property ones.

Ah, nice catch! I didn't realize the same node type was used in both cases.

nzakas and others added 3 commits November 19, 2024 10:44
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
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!

@mdjermanovic mdjermanovic merged commit 9b80bdd into main Nov 20, 2024
15 checks passed
@mdjermanovic mdjermanovic deleted the no-valid-properties-combined branch November 20, 2024 06:41
@github-actions github-actions bot mentioned this pull request Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants