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(eslint-plugin): [class-methods-use-this] add extension rule #6457
Conversation
Thanks for the PR, @bradzacher! 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. |
@bradzacher is this still something you have time to work on? |
I think it was almost done if memory serves me correctly. I'll have a look today. |
71620da
to
b110ee9
Compare
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 41da796. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 35 targets
Sent with 💌 from NxCloud. |
b110ee9
to
04b4d2c
Compare
04b4d2c
to
4527fb7
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## update-getFunctionHeadLoc #6457 +/- ##
=============================================================
- Coverage 87.48% 87.40% -0.09%
=============================================================
Files 379 381 +2
Lines 13250 13311 +61
Branches 3911 3933 +22
=============================================================
+ Hits 11592 11634 +42
- Misses 1279 1292 +13
- Partials 379 385 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
> 🛑 This file is source code, not the primary documentation location! 🛑 | ||
> | ||
> See **https://typescript-eslint.io/rules/your-rule-name** for documentation. | ||
> See **https://typescript-eslint.io/rules/RULE_NAME_REPLACEME** for documentation. |
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.
unrelated?
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.
it was just me fleshing out the template
i noticed this was super easy to miss this so I made it more obvious!
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 like 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.
Super! ✨
ACK on the pulling in code from upstream as-is. I trust you on filling in the missing unit tests coverage if needed for the rule.
pushContext(); | ||
}, | ||
'FunctionDeclaration:exit'(): void { | ||
popContext(); |
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.
[Testing] Was it intentional that these bits aren't covered by unit 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.
I've got all the tests from the base rule - so either it's not covered by the base tests or code cov is wrong again. IDK
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 ESLint core isn't testing these lines, who are we to dispute??
The base branch was changed.
PR Checklist
Overview
This adds a standard AST fork of the base rule that can be configured to ignore one or both of the cases where a method has the
override
keyword, or the parent class implements an interface.Based on #7260