-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: add no-property-in-node rule #433
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you!
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anall-type-checked
config? - Disable
no-property-in-node
for specificallyeslint/eslint
, since it's the only one without a TSConfig?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.😅
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Continuing discussion in:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks! 💯
| ✅ | `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 | |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dangit, sorry, copy & paste
Fixes #326.