fix(eslint-plugin): [prefer-readonly] report if a member's property is reassigned #6043
Conversation
|
Thanks for the PR, @islandryu! 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 settings. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6043 +/- ##
=======================================
Coverage 91.27% 91.28%
=======================================
Files 366 366
Lines 12362 12363 +1
Branches 3616 3617 +1
=======================================
+ Hits 11284 11285 +1
Misses 768 768
Partials 310 310
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Code change looks great to start, thanks for sending this in @islandryu! 🙌 Just requesting changes for a bit more test coverage.
| messageId: 'preferReadonly', | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
A few more test cases to consider...
this.testObj.prop;
this.testObj?.prop;
this.testObj!.prop;
this.testObj!.prop = '';
this.testObj.prop.prop = '';
this.testObj.prop.doesSomething();
this.testObj?.prop.prop;
this.testObj?.prop?.prop;
this.testObj.prop?.prop;
this.testObj!.prop?.prop;For any change to rule logic, it's generally good to try to add both valid and invalid test cases.
There was a problem hiding this comment.
@islandryu by "test cases" I do mean cases plural - we generally try to keep these separate, so that each test case tests one case. It's easy to miss incorrect or mismatched errors when they're all lumped together. Could you split them up please?
|
@JoshuaKGoldberg |
| messageId: 'preferReadonly', | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
@islandryu by "test cases" I do mean cases plural - we generally try to keep these separate, so that each test case tests one case. It's easy to miss incorrect or mismatched errors when they're all lumped together. Could you split them up please?
|
@JoshuaKGoldberg |
PR Checklist
Overview