-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat: support TypeScript syntax in no-empty-function rule #19551
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
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
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.
LGTM.
I'll leave it open for others to review.
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 really close to me, just a couple touchups. 🙌
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 behavior of new options would be different than the options of the @typescript-eslint/no-empty-function
rule.
For example, this code is currently valid for both core no-empty-function
and @typescript-eslint/no-empty-function
rules:
/* eslint @typescript-eslint/no-empty-function: ["error", { "allow": ["constructors", "methods"] }] */
/* eslint no-empty-function: ["error", { "allow": ["constructors", "methods"] }] */
class A {
private constructor() { }
}
class B {
protected constructor() { }
}
class C {
@decorator
foo() { }
}
class D extends C {
override foo() { }
}
But after this change, all 4 functions would be invalid because they're no longer considered to be of constructors
/methods
kind, which I'm not sure is desirable and might be considered a breaking change.
@mdjermanovic That was definitely an oversight. I've fixed it and added tests. |
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.
LGTM, thanks! Leaving open for @snitin315 and @JoshuaKGoldberg to re-review.
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.
LGTM, tahnks 🙌🏻.
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 source code looks very good and reasonable to me, nice! Requesting information from the team on what the stance on the >2k newly generated tests are, not blocking.
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.
If everybody else is happy, then I'm happy too
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] 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)
Added support for handling TypeScript specific code that would otherwise trigger the rule.
One example of valid TypeScript specific code that would otherwise trigger the no-empty-function rule is the use of parameter properties in constructor functions.
This PR also adds four new options to the allow list:
"privateConstructors"
"protectedConstructors"
"decoratedFunctions"
"overrideMethods"
Is there anything you'd like reviewers to focus on?
Refs #19173.