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

Jump to bottom

Update: check computed keys in no-prototype-builtins (fixes #13088) #13755

Merged
merged 1 commit into from Oct 19, 2020
Merged

Update: check computed keys in no-prototype-builtins (fixes #13088) #13755

merged 1 commit into from Oct 19, 2020

Conversation

mdjermanovic
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an 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 #13088

What changes did you make? (Give an overview)

Changed no-prototype-builtins rule to check computed property access and report simple cases like foo["hasOwnProperty"]("bar").

Also refactored the test file so that the tests can be checked by eslint-plugin-eslint-plugin rules.

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

The removed TODO suggested to just replace all lines with astUtils.getStaticPropertyName(node.callee) but I think it's more correct to explicitly check for MemberExpression. Also, astUtils.skipChainExpression would be anyway needed to get callee.property.loc.

@mdjermanovic mdjermanovic added 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 Oct 13, 2020
kaicataldo
kaicataldo approved these changes Oct 13, 2020
View reviewed changes
Copy link
Member

@kaicataldo kaicataldo 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!

yeonjuan
yeonjuan approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@yeonjuan yeonjuan left a comment

Choose a reason for hiding this comment

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

LGTM!

@mdjermanovic mdjermanovic merged commit 67c0605 into master Oct 19, 2020
@mdjermanovic mdjermanovic deleted the issue13088 branch October 19, 2020 10:48
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 18, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kaicataldo kaicataldo kaicataldo approved these changes

@yeonjuan yeonjuan yeonjuan 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 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.

use astUtils.getStaticPropertyName in no-prototype-builtins
3 participants
@mdjermanovic @kaicataldo @yeonjuan