Skip to content
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

Update: no-self-assign should detect member expression with this #12279

Merged
merged 2 commits into from Oct 15, 2019

Conversation

@saberduck
Copy link
Contributor

@saberduck saberduck commented Sep 17, 2019

What is the purpose of this pull request? (put an "X" next to item)

[x] Changes an existing rule

What rule do you want to change?
no-self-assign

Does this change cause the rule to produce more or fewer warnings?
more

How will the change be implemented? (New option, new default behavior, etc.)?
updates default behavior

Please provide some example code that this change will affect:

this.x = this.x

What does the rule currently do for this code?
doesn't raise an issue

What will the rule do after it's changed?
raise an issue

What changes did you make? (Give an overview)
I updated no-self-assign rule to support member expression using this identifier.

Is there anything you'd like reviewers to focus on?

@eslint-deprecated eslint-deprecated bot added the triage label Sep 17, 2019
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 17, 2019

Hi @saberduck, thanks for the PR!

Seems logical to report this as it's a self-assignment of a property.

Though, this needs to be evaluated for a semver-minor upgrade because the change produces more warnings by default.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Sep 18, 2019

Wondering if this can be handled as a bug fix, as I believe there is no fundamental difference between obj.x = obj.x and this.x = this.x regarding the purpose and the logic of this rule.

On the other hand, it could break some builds where this line is used to trigger some setter's side effects.

@saberduck
Copy link
Contributor Author

@saberduck saberduck commented Oct 14, 2019

@mdjermanovic can I help somehow to move this PR forward? Should I change the description to bugfix?

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Oct 14, 2019

I think this could be considered a bug. (Reproduction demo)

Based on that, I will mark as accepted/bug.

NOTE: The current commit message is correct, as we want bug fixes that will report more errors to be marked with the "Update:" prefix and included in a minor release.

@platinumazure
Copy link
Member

@platinumazure platinumazure commented Oct 14, 2019

Also, please accept my apologies for letting this sit for so long.

Copy link
Member

@platinumazure platinumazure left a comment

Looks good to me. Thanks for contributing!

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, just one note.

lib/rules/no-self-assign.js Outdated Show resolved Hide resolved
@saberduck saberduck force-pushed the saberduck:fix_self_assign_this branch from a773a61 to 3fd7758 Oct 15, 2019
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Oct 15, 2019

Thanks for contributing!

@platinumazure platinumazure merged commit b962775 into eslint:master Oct 15, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191015.2 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@platinumazure
Copy link
Member

@platinumazure platinumazure commented Oct 15, 2019

Thanks @saberduck for contributing to ESLint!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants