typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star16k

fix(eslint-plugin): [prefer-readonly] report if a member's property is reassigned#6043

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

JoshuaKGoldberg merged 6 commits intotypescript-eslint:mainfromislandryu:fix/prefer-readonly-member-property

Nov 26, 2022

Merged

fix(eslint-plugin): [prefer-readonly] report if a member's property is reassigned#6043

JoshuaKGoldberg merged 6 commits intotypescript-eslint:mainfromislandryu:fix/prefer-readonly-member-property

Nov 26, 2022

+349 −1

Conversation

Copy link

Contributor

islandryu commentedNov 19, 2022•
edited
Loading

PR Checklist

Addresses an existing open issue:fixesBug: [prefer-readonly] does not report if a member's property is reassigned #6005
That issue was marked asaccepting prs
Steps inCONTRIBUTING.md were taken

Overview

fix(eslint-plugin): [prefer-readonly] report if a member's property i…

3d88d10

…s reassigned

Copy link

nx-cloudbot commentedNov 19, 2022•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commitbe983e9. 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 47 targets

Sent with 💌 fromNxCloud.

Copy link

Contributor

typescript-eslintbot commentedNov 19, 2022

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.

Copy link

netlifybot commentedNov 19, 2022•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

Name	Link
🔨 Latest commit	`be983e9`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6382776df71c450008c5c1f5
😎 Deploy Preview	https://deploy-preview-6043--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to yourNetlify site settings.

format

94c6b98

Copy link

codecovbot commentedNov 19, 2022•
edited
Loading

Codecov Report

Merging#6043 (be983e9) intomain (d69fdf4) willincrease coverage by0.00%.
The diff coverage is100.00%.

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

Flag	Coverage Δ
unittest	`91.28% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown.Click here to find out more.

Impacted Files	Coverage Δ
...ackages/eslint-plugin/src/rules/prefer-readonly.ts	`99.09% <100.00%> (+<0.01%)`	⬆️

islandryu marked this pull request as ready for review

November 19, 2022 13:17

JoshuaKGoldberg requested changes

Nov 19, 2022

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Code change looks great to start, thanks for sending this in@islandryu! 🙌 Just requesting changes for a bit more test coverage.

packages/eslint-plugin/tests/rules/prefer-readonly.test.ts

		messageId:'preferReadonly',
		},
		],
		},

Copy link

Member

JoshuaKGoldbergNov 19, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 bothvalid andinvalid test cases.

Copy link

Member

JoshuaKGoldbergNov 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@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 added the awaiting responseIssues waiting for a reply from the OP or another party label

Nov 19, 2022

Add test case

8a147ec

Copy link

ContributorAuthor

islandryu commentedNov 21, 2022

@JoshuaKGoldberg
Thank you!
I've added a test case!

islandryu requested a review fromJoshuaKGoldberg

November 22, 2022 22:31

JoshuaKGoldberg requested changes

Nov 23, 2022

View reviewed changes

Copy link

Member

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sorry, I might not have been very clear - I'm hoping for quite a few cases to test this. It's a behavior that has a lot of edge cases we should test for individually. Thanks!