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: test-async-wrap-constructor refactor-for-to-foreach #32631

Closed
wants to merge 4 commits into from
Closed

test: test-async-wrap-constructor refactor-for-to-foreach #32631

wants to merge 4 commits into from

Conversation

dericop
Copy link
Contributor

@dericop dericop commented Apr 3, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [x ] tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 3, 2020
@addaleax
Copy link
Member

addaleax commented Apr 3, 2020

Hi @dericop! Can you make sure that make lint passes?

That being said, I think it’s more common for new code to use for (... of ...) than .forEach() in our codebase.

@dericop
Copy link
Contributor Author

dericop commented Apr 3, 2020

@addaleax you're right. I've been reading and for/of is the most robust way to iterate over an array in JavaScript.

@jasnell
Copy link
Member

jasnell commented Apr 3, 2020

That being said, I think it’s more common for new code to use for (... of ...) than .forEach() in our codebase.

For this kind of thing, using .forEach() is fairly common in our tests and not very problematic.

@dericop @juanarbol
Update message in test/parallel/test-async-wrap-constructor.js
24c4fd4 
Co-Authored-By: Juan José <soyjuanarbol@gmail.com>
@dericop @juanarbol
fix message in test/parallel/test-async-wrap-constructor.js
0dbe2cf 
Co-Authored-By: Juan José <soyjuanarbol@gmail.com>
@dericop
Copy link
Contributor Author

dericop commented Apr 5, 2020

@addaleax Do you think that i should make any changes for approved the pull request?

@addaleax
Copy link
Member

addaleax commented Apr 5, 2020

@dericop That question probably doesn’t go to just me. ;)

I’ll approve this, with the error message test this seems like a good addition 👍

@Trott
Copy link
Member

Trott commented Apr 13, 2020

When this is landed, the commit message should be edited to conform with doc/guides/contributing/pull-requests.md (start with an imperative verb, etc.). Maybe this?

test: use forEach() in test-async-wrap-constructor

@juanarbol juanarbol added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 23, 2020
@nodejs-github-bot
Copy link
Collaborator

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

juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 24, 2020
@dericop @juanarbol
test: test-async-wrap-constructor prefer forEach
e99ec39 
PR-URL: nodejs#32631
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@juanarbol
Copy link
Member

Landed in e99ec39 🎉

Thank you!! Amazing

@juanarbol juanarbol closed this Apr 24, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
@dericop @BethGriggs
test: test-async-wrap-constructor prefer forEach
29f7c4d 
PR-URL: #32631
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BethGriggs BethGriggs mentioned this pull request Apr 27, 2020
Merged
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
@dericop @BridgeAR
test: test-async-wrap-constructor prefer forEach
e6e0647 
PR-URL: #32631
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
@dericop @BridgeAR
test: test-async-wrap-constructor prefer forEach
c3025f3 
PR-URL: #32631
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Apr 28, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
@dericop @BethGriggs
test: test-async-wrap-constructor prefer forEach
b4ef062 
PR-URL: #32631
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Apr 30, 2020
@dericop @targos
test: test-async-wrap-constructor prefer forEach
a41d4d0 
PR-URL: #32631
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request May 2, 2020
Merged
targos pushed a commit that referenced this pull request May 13, 2020
@dericop @targos
test: test-async-wrap-constructor prefer forEach
4c67568 
PR-URL: #32631
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himself65 himself65 himself65 left review comments

@juanarbol juanarbol juanarbol approved these changes

@addaleax addaleax addaleax 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. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dericop @addaleax @jasnell @Trott @nodejs-github-bot @juanarbol @himself65