-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix(eslint-plugin): [no-extraneous-class] handle abstract members #9367
Conversation
Thanks for the PR, @yoshi2no! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 66770d2. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
no-extraneous-class
ruleThere 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.
Congrats on a great first PR to typescript-eslint!
AFAICT, everything that you have so far is perfect! The changes requested are just for a few finishing touches 🙂 Only blocker for me is the request around testing.
Thanks!
Also, TIL that |
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.
Great stuff! Just one nit remaining
@@ -44,6 +44,10 @@ class HelloWorldLogger { | |||
} | |||
``` | |||
|
|||
```ts |
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.
nit - please include these in the preceeding tabs, rather than making a new one
FYI - to fix the CI failure, you'll need to update the docs snapshots. See https://typescript-eslint.io/contributing/local-development#rule-development |
Oops, I missed that. Thanks! |
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.
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.
Yay!
PR Checklist
Overview
Description
This PR addresses an issue with the @typescript-eslint/no-extraneous-class rule where abstract fields and methods were not being counted as instance properties. The rule implementation was missing the handling of abstract members, which have different AST nodes compared to their concrete counterparts. This fix ensures that abstract fields and methods are correctly recognized as instance properties.
Changes
TSAbstractMethodDefinition
andTSAbstractPropertyDefinition
nodes.