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: add no-property-in-node rule #433

Merged

Conversation

JoshuaKGoldberg
Copy link
Contributor

Fixes #326.

README.md Outdated Show resolved Hide resolved
tests/lib/rules/no-property-in-node.js Outdated Show resolved Hide resolved
lib/rules/no-property-in-node.js Outdated Show resolved Hide resolved
docs/rules/no-property-in-node.md Outdated Show resolved Hide resolved
docs/rules/no-property-in-node.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
tests/lib/rules/no-property-in-node.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@bmish
Copy link
Member

bmish commented Feb 6, 2024

@aladdin-add let's release this as a minor version. I'll leave it for you to review or merge and then conduct the minor release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From https://github.com/eslint-community/eslint-plugin-eslint-plugin/actions/runs/7794182409/job/21255096639?pr=433:

Results:
Repository: eslint/eslint
Rule: eslint-plugin/no-property-in-node
Message: You have used a rule which requires parserServices to be generated. You must therefore provide a value for the "parserOptions.project" property for @typescript-eslint/parser.
Occurred while linting /home/runner/work/eslint-plugin-eslint-plugin/eslint-plugin-eslint-plugin/node_modules/eslint-remote-tester/.cache-eslint-remote-tester/eslint/eslint/docs/src/assets/js/focus-visible.js:41
Rule: "eslint-plugin/no-property-in-node"
Path: eslint/eslint/docs/src/assets/js/focus-visible.js
Link: https://github.com/eslint/eslint/blob/HEAD/docs/src/assets/js/focus-visible.js#L41

What would you like to do here? The first ideas that come to mind for me are:

  • Disable type-checked rules in all & add an all-type-checked config?
  • Disable no-property-in-node for specifically eslint/eslint, since it's the only one without a TSConfig?

Copy link
Contributor

Choose a reason for hiding this comment

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

Disable type-checked rules in all & add an all-type-checked config?

I slightly prefer this. just likerecommended and recommended-type-checked. @bmish wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

the rule is also rules-recommended? So also need rules-recommended-type-checked...., so many presets doesn't seem like a good idea.😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah 😓 ... another possibility for flat configs specifically could be to make the configs be a function with properties set on it. So you could do either of:

export default [
  eslintPlugin.configs.recommended,
  eslintPlugin.configs.recommended({ typeChecked: true }),
]

The only prior art I know of this strategy is azat-io/eslint-plugin-perfectionist#90.

Copy link
Contributor

Choose a reason for hiding this comment

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

recommended config follows the eslint's semantic-versioning-policy, while all, rules-all, tests-all does not.

maybe just adding recommended: false in this PR. let's consider adding the configs in another PR. thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Duplicating every config when we have so many is not great. What about just providing a type-checked config with just this single rule that you can mix and match with the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, that's what we used to do in typescript-eslint. But then we found that most folks only enabled one of the configs. And it was a bit irksome to have to enable two.

typescript-eslint/typescript-eslint#5204 -> https://typescript-eslint.io/blog/announcing-typescript-eslint-v6#reworked-configuration-names is what we ended up with.

Copy link
Member

Choose a reason for hiding this comment

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

Continuing discussion in:

Copy link
Contributor

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! 💯

@aladdin-add aladdin-add merged commit d2b9372 into eslint-community:main Feb 6, 2024
5 of 6 checks passed
@github-actions github-actions bot mentioned this pull request Feb 6, 2024
@JoshuaKGoldberg JoshuaKGoldberg deleted the no-property-in-node branch February 6, 2024 14:11
| ✅ | `recommended` | enables all recommended rules in this plugin |
| | `rules-recommended` | enables all recommended rules that are aimed at linting ESLint rule files |
| | `tests-recommended` | enables all recommended rules that are aimed at linting ESLint test files |
| | `all` | enables all rules in this plugin, including those requiring type information |
Copy link
Member

Choose a reason for hiding this comment

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

looks like this should be excluding

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dangit, sorry, copy & paste

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule request: don't use the in operator on nodes
4 participants