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.

Sign up for GitHub

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

Jump to bottom

Update: Let no-restricted-properties check destructuring (fixes #7147) #7151

Merged
merged 1 commit into from
Sep 19, 2016
Merged

Update: Let no-restricted-properties check destructuring (fixes #7147) #7151

merged 1 commit into from
Sep 19, 2016

Conversation

not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Sep 14, 2016 •
edited

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

[ ] Documentation update
[x] Bug fix ([template](https://github.com/eslint/eslint/blob/master/templates/bug-report.md))
[ ] New rule ([template](https://github.com/eslint/eslint/blob/master/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://github.com/eslint/eslint/blob/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

See #7147 for the template answers.

Please check each item to ensure your pull request is ready:

  • I've read the pull request guide
  • I've included tests for my change
  • I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)

This allows no-restricted-properties to report errors from destructuring patterns.

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

Nothing in particular.

This PR is built on top of the commit from #7139, since it includes some test cases that correspond to the options in #7139. It shouldn't be merged until after #7139 is merged. (edit: #7139 is merged now.)

In the meantime, see here for the diff that only includes changes from this PR.

@mention-bot
Copy link

@not-an-aardvark, thanks for your PR! By analyzing the annotation information on this pull request, we identified @TheSavior and @alecharmon to be potential reviewers

@eslintbot
Copy link

LGTM

mysticatea
mysticatea reviewed Sep 14, 2016
View reviewed changes
lib/rules/no-restricted-properties.js
const message = matchedObjectProperty.message ? " " + matchedObjectProperty.message : "";
checkPropertyAccess(node, objectName, propertyName);
},
VariableDeclarator(node) {
Copy link
Member

Choose a reason for hiding this comment

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

And AssignmentExpression/AssignmentPattern can access properties by destructuring.

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the PR to check for an AssignmentExpression, but what is an example of an AssignmentPattern?

Copy link
Member

Choose a reason for hiding this comment

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

const obj = {a: 1}
function foo({a} = obj) { // obj.a
}

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 to check for AssignmentPatterns as well.

Copy link
Member

Choose a reason for hiding this comment

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

@mysticatea does this rule flag this?

function foo(obj) {
    return obj.a;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@nzakas: Yes; it reports any reference to the a property of anything named obj (assuming the config is [2, {"object": "obj", "property": "a"}] ).

Copy link
Member

Choose a reason for hiding this comment

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

Cool, just wanted to double-check. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Yes. It's checked as a MemberExpression.

@eslintbot
Copy link

LGTM

@eslintbot
Copy link

LGTM

@not-an-aardvark
Copy link
Member Author

not-an-aardvark commented Sep 14, 2016 •
edited

On second thought, I realized maybe we shouldn't consider var [bar] = foo to be accessing the 0 property of foo since that's only true for arrays. It would be a false positive for any other iterable:

/* eslint no-restricted-properties: [2, {"object": "foo", "property": "0"}] */

var foo = new Set();

var [bar] = foo; // reports an error with this PR, but it probably shouldn't.

So we should only consider ObjectPattern destructuring and not ArrayPattern.

edit: Updated this PR to only account for ObjectPattern destructuring.

@eslintbot
Copy link

LGTM

@nzakas nzakas added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 14, 2016
@platinumazure
Copy link
Member

@not-an-aardvark We can consider supporting array destructure later-- I think supporting object destructure is the most important thing here.

mysticatea
mysticatea suggested changes Sep 17, 2016
View reviewed changes
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 except above comment.

lib/rules/no-restricted-properties.js
});
if (node.id.type === "ObjectPattern") {
node.id.properties.forEach(property => {
checkPropertyAccess(node.id, objectName, property.key.name || property.key.value);
Copy link
Member

Choose a reason for hiding this comment

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

This should ignore the property if the property has dynamic name.

const name = astUtils.getStaticPropertyName(property);
if (name) {
    checkPropertyAccess(node.id, objectName, name);
}

There is the same code in checkDestructuringAssignment function.

@mysticatea mysticatea added bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Sep 17, 2016
@eslintbot
Copy link

LGTM

mysticatea
mysticatea reviewed Sep 17, 2016
View reviewed changes
lib/rules/no-restricted-properties.js
}
},
additionalProperties: false,
required: ["object"]
Copy link
Member

Choose a reason for hiding this comment

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

By the way, I'm wondering if property should be omittable because this rule is no-restricted-properties

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, never mind...

@mysticatea
Copy link
Member

LGTM, thank you!
Just waiting for #7139 is accepted.

@eslintbot
Copy link

LGTM

@nzakas nzakas merged commit c118d21 into eslint:master Sep 19, 2016
@not-an-aardvark not-an-aardvark deleted the issue7147 branch September 19, 2016 18:12
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mysticatea mysticatea mysticatea approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@not-an-aardvark @mention-bot @eslintbot @platinumazure @mysticatea @nzakas @jquerybot