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-useless-rename also reports default values (fixes #12301) #12322

Merged
merged 2 commits into from Sep 29, 2019

Conversation

kaicataldo
Copy link
Member

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

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

fixes #12301

What changes did you make? (Give an overview)

This updates no-useless-rename to check destructured assignments with default values. I think this should be considered a semver-minor bug fix, since it feels like an oversight that this rule doesn't already check this case.

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

Are reviewers in agreement that this should be default behavior and not behind an option?

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Sep 27, 2019
@kaicataldo kaicataldo added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Sep 27, 2019
@kaicataldo kaicataldo added the enhancement This change enhances an existing feature of ESLint label Sep 27, 2019
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I agree this should be semver-minor.

@@ -85,7 +90,9 @@ module.exports = {
const properties = node.properties;

for (let i = 0; i < properties.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we use the index anymore, outside of the next line? If not, should this be turned into a for-of loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated! Did some further refactoring while I was at it. The code looks a bit simpler now!

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

*/
if (properties[i].computed || !properties[i].key) {
if (property.shorthand || property.type === "RestElement" || property.computed) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like there are already tests covering the property.type === "RestElement" change on this line.

@platinumazure platinumazure merged commit 334ca7c into master Sep 29, 2019
@platinumazure platinumazure deleted the fixes12301 branch September 29, 2019 04:03
if (properties[i].key.type === "Identifier" && properties[i].key.name === properties[i].value.name ||
properties[i].key.type === "Literal" && properties[i].key.value === properties[i].value.name) {
reportError(properties[i], properties[i].key, properties[i].value, "Destructuring assignment");
const key = (property.key.type === "Identifier" && property.key.name) || (property.key.type === "Literal" && property.key.value);
Copy link

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Please track this issue: #12335 .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-useless-rename should also report pattern properties with default values
5 participants