-
-
Notifications
You must be signed in to change notification settings - Fork 634
feat(linter): add eslint/class-methods-use-this rule #12977
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
base: main
Are you sure you want to change the base?
feat(linter): add eslint/class-methods-use-this rule #12977
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
CodSpeed Instrumentation Performance ReportMerging #12977 will not alter performanceComparing Summary
|
6926f50
to
b548897
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.
I think we can simplify the implementation a bit. I’ve left two comments for changes, and need @camc314 to confirm them.
@huangtiandi1999 fixed in latest commit! code is much easier to reason about now and was able to figure out the last few test cases as well. |
b1c8e83
to
a422933
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.
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.
Pull Request Overview
This PR implements the ESLint class-methods-use-this
rule for the Oxc linter. The rule enforces that class methods utilize the this
keyword, helping identify methods that could be converted to static methods for better performance and clarity.
Key changes implemented:
- Added comprehensive rule logic with configuration options for method exceptions, class field enforcement, override method handling, and interface implementation handling
- Implemented AST visitor pattern to detect
this
usage within method bodies while properly handling scope boundaries - Added extensive test coverage with 422 test cases covering various method types and edge cases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
crates/oxc_linter/src/rules/eslint/class_methods_use_this.rs |
Main rule implementation with configuration parsing, AST analysis, and comprehensive test suite |
crates/oxc_linter/src/rules.rs |
Module registration for the new rule |
crates/oxc_linter/src/snapshots/eslint_class_methods_use_this.snap |
Snapshot test output showing diagnostic messages for various failing test cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Adds the
class-methods-use-this
rule.Added a bunch of tests from the eslint repo and the typescript eslint repo, without the ones that rely on particular option configurations.
Works towards #479