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

Fix: prefer-const when using destructuring assign (fixes #8308) #10924

Merged
merged 8 commits into from Oct 19, 2018

Conversation

@nzakas
Copy link
Member

commented Oct 5, 2018

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

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Changed prefer-const so that it considers destructuring assignment with an object property to disqualify using const for variables used in the same destructuring assignment.

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

Is this the correct approach? It seems like the key to fixing this bug was to consider object properties in destructuring assignment to be the key factor in determining if a variable should be declared with const. I'm not 100% this is the correct assumption, so please double-check that.

@eslint eslint bot added the triage label Oct 5, 2018

@nzakas nzakas requested a review from mysticatea Oct 5, 2018

@nzakas nzakas added bug rule and removed triage labels Oct 5, 2018

@platinumazure
Copy link
Member

left a comment

LGTM so far, but was unsure about a few possible typos/unintentional changes.

tests/lib/rules/prefer-const.js Outdated Show resolved Hide resolved
tests/lib/rules/prefer-const.js Show resolved Hide resolved
tests/lib/rules/prefer-const.js Outdated Show resolved Hide resolved

nzakas added some commits Oct 6, 2018

@nzakas

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

@mysticatea if you have time, can you take a look at this pull request?

@not-an-aardvark
Copy link
Member

left a comment

Thanks! I noticed one potential bug, but this generally looks like a good approach.

lib/rules/prefer-const.js Show resolved Hide resolved
@nzakas

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

@not-an-aardvark
Copy link
Member

left a comment

LGTM, thanks!

lib/rules/prefer-const.js Outdated Show resolved Hide resolved
@kaicataldo
Copy link
Member

left a comment

LGTM. Thanks for working on this!

@kaicataldo kaicataldo merged commit 57ef0fd into master Oct 19, 2018

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details

@nzakas nzakas deleted the issue8308 branch Oct 19, 2018

@eslint eslint bot locked and limited conversation to collaborators Apr 18, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.