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

test: use Object.hasOwn() where applicable #41664

Merged
merged 1 commit into from
Jan 25, 2022
Merged

test: use Object.hasOwn() where applicable #41664

merged 1 commit into from
Jan 25, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 23, 2022

Replace Object.prototpye.hasOwnProperty() with Object.hasOwn() where
applicable.

@Trott
test: use Object.hasOwn() where applicable
797c71b 
Replace Object.prototpye.hasOwnProperty() with Object.hasOwn() where
applicable.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jan 23, 2022
@Trott Trott added dont-land-on-v12.x request-ci Add this label to start a Jenkins CI on a PR. and removed doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. needs-ci PRs that need a full CI run. labels Jan 23, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 23, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42095/

benjamingr
benjamingr approved these changes Jan 23, 2022
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Sure, but why? I get the value of hasOwn when interacting with objects whose prototype you can't be certain of but these are our own tests

aduh95
aduh95 approved these changes Jan 23, 2022
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

The goal is to enforce it with a lint rule in the future? I'm all for it.

@Trott
Copy link
Member Author

Trott commented Jan 23, 2022 •
edited
Loading

Sure, but why? I get the value of hasOwn when interacting with objects whose prototype you can't be certain of but these are our own tests

The goal is to enforce it with a lint rule in the future? I'm all for it.

These are all flagged by the no-prototype-builtins rule which is included in the eslint:recommended configuration and which we then explicitly disable. I would like to remove that exception and enable the rule. These account for most of the code flagged, but there are still a few others.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42106/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42111/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42114/

@github-actions github-actions bot mentioned this pull request Jan 24, 2022
Open
22 tasks
bnb
bnb approved these changes Jan 24, 2022
View reviewed changes
Copy link
Contributor

@bnb bnb left a comment

Choose a reason for hiding this comment

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

:))))))))))))

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 24, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/42122/

@github-actions github-actions bot mentioned this pull request Jan 25, 2022
Open
28 tasks
@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 25, 2022
@nodejs-github-bot nodejs-github-bot merged commit e2e2bc8 into nodejs:master Jan 25, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in e2e2bc8

@Trott Trott deleted the hasown branch January 25, 2022 15:00
This was referenced Jan 26, 2022
Open
Open
Open
Open
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@Trott @Linkgoron
test: use Object.hasOwn() where applicable
b61d014 
Replace Object.prototpye.hasOwnProperty() with Object.hasOwn() where
applicable.

PR-URL: nodejs#41664
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
@Trott @ruyadorno
test: use Object.hasOwn() where applicable
0267d27 
Replace Object.prototpye.hasOwnProperty() with Object.hasOwn() where
applicable.

PR-URL: #41664
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Feb 8, 2022
Merged
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
@Trott @danielleadams
test: use Object.hasOwn() where applicable
4423757 
Replace Object.prototpye.hasOwnProperty() with Object.hasOwn() where
applicable.

PR-URL: #41664
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@danielleadams danielleadams mentioned this pull request Mar 3, 2022
Merged
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
@Trott @danielleadams
test: use Object.hasOwn() where applicable
a0da7a0 
Replace Object.prototpye.hasOwnProperty() with Object.hasOwn() where
applicable.

PR-URL: #41664
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
@Trott @danielleadams
test: use Object.hasOwn() where applicable
639130e 
Replace Object.prototpye.hasOwnProperty() with Object.hasOwn() where
applicable.

PR-URL: #41664
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mesteery Mesteery Mesteery approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@aduh95 aduh95 aduh95 approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@addaleax addaleax addaleax approved these changes

@bnb bnb bnb approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Trott @nodejs-github-bot @bnb @addaleax @benjamingr @lpinca @aduh95 @RaisinTen @Mesteery