-
- Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(eslint-plugin): [class-methods-use-this] detect a problematic case for private/protected members if ignoreClassesThatImplementAnInterface is set #7705
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
| Thanks for the PR, @tetsuharuohzeki! 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. |
4d4e714 to d4b5aaf Compare | Converting to draft since the linked issue hasn't been accepted yet. Let's continue conversation there. |
80b3efe to 3625d70 Compare 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.
OK! This looks great to me - just one docs suggestion from me. What do you think @tetsuharuohzeki?
Thanks for waiting btw - I'm excited about this option!
| | 'public-fields' | ||
| /** Ignore classes that specifically implement some interface */ |
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.
Hmm, this looks a little off... outside the scope of this PR.
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.
I tried to remove its comment but I seem nx run eslint-plugin:test generate it...
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.
I tried to add more descriptions but it cannot remove extra comments.
7390468
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 I think this is just something odd in the repository, separate from your changes. No worries. Thanks for trying though!
3625d70 to 7390468 Compare 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.
Swell, thanks!
| I introduced a merge conflict on |
…ass private field
…matic case for private/protected members if `ignoreClassesThatImplementAnInterface` is set
ignoreClassesThatImplementAnInterface is trueignoreClassesThatImplementAnInterface is set 42e2148 to b7beb57 Compare | Thank you for your review! I rebase my change sets on the latest main! |
| Amazing, thanks! Saved me a bit of time wrestling with it locally. 🙂 |
| @JoshuaKGoldberg Thank you for accepting my patch. |

PR Checklist
ignoreClassesThatImplementAnInterfaceoption istrue#7704Overview
This fixes #7704.
TypeScript's interface feature does not have any private/protected member as a part of it.
So the rule
class-methods-use-thisenabling ignoreClassesThatImplementAnInterface=true should warn the case of that the implementation is not a part of interface.