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: false negative with onlyDeclarations + properties in id-match #15431

Merged
merged 5 commits into from
Dec 21, 2021

Conversation

snitin315
Copy link
Contributor

@snitin315 snitin315 commented Dec 16, 2021

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fixes #15123

Report invalid property names for ObjectExpressions when property: true, regardless the value of onlyDeclaration option.

Before - Online Demo

After

/* eslint "id-match": [
      "error",
      "^[^_]+$",
      {
            "properties": true,
            "onlyDeclarations": true
      }
] */

const foo = {
                foo_one: 1, // error
                bar_one: 2, // error
                fooBar: 3 // ok
     };

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

No

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon bug ESLint is working incorrectly labels Dec 16, 2021
@snitin315 snitin315 force-pushed the fix/id-match-only-declarations branch from 694e90c to 8dad653 Compare December 16, 2021 11:04
@snitin315 snitin315 changed the title fix: false negative with onlyDeclarations in id-match fix: false negative with onlyDeclarations + properties in id-match Dec 16, 2021
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Dec 17, 2021
@mdjermanovic mdjermanovic changed the title fix: false negative with onlyDeclarations + properties in id-match feat: false negative with onlyDeclarations + properties in id-match Dec 17, 2021
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Dec 17, 2021
@mdjermanovic
Copy link
Member

I changed the title to feat: because this change produces more warnings, and then also removed a pair of backticks to fit the maximum of 72 characters.

lib/rules/id-match.js Outdated Show resolved Hide resolved
lib/rules/id-match.js Outdated Show resolved Hide resolved
Comment on lines 240 to 251
{
code: `
const foo = {
[a]: 1,
};
`,
options: ["^[^a]", {
properties: false,
onlyDeclarations: false
}],
parserOptions: { ecmaVersion: 2022 }
},
Copy link
Member

Choose a reason for hiding this comment

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

This should be invalid, but that's another bug. It might be best to just remove this test case for now, and revisit this rule at some point. In particular, L219-L249 looks broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should create a separate issue to track it.

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 9d6fe5a into main Dec 21, 2021
@mdjermanovic mdjermanovic deleted the fix/id-match-only-declarations branch December 21, 2021 14:27
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 20, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐞 BUG: id-match rule not working properly with both properties & onlyDeclarations options enabled
2 participants