FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: test-async-wrap-constructor refactor-for-to-foreach by dericop · Pull Request #32631 · nodejs/node · GitHub

/ node Public

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

Closed
dericop wants to merge 4 commits into
nodejs:masterfrom
dericop:test-async-refactor-for-to-foreach
ConversationCommits (4)ChecksFiles changed
Closed

test: test-async-wrap-constructor refactor-for-to-foreach#32631
dericop wants to merge 4 commits into
nodejs:masterfrom
dericop:test-async-refactor-for-to-foreach

Conversation

dericop commented Apr 3, 2020

Copy link
Copy Markdown
Contributor
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 added the test Issues and PRs related to the tests. label Apr 3, 2020

addaleax commented Apr 3, 2020

Copy link
Copy Markdown
Member

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 commented Apr 3, 2020

Copy link
Copy Markdown
Contributor Author

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

jasnell commented Apr 3, 2020

Copy link
Copy Markdown
Member

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.

Update message in test/parallel/test-async-wrap-constructor.js
24c4fd4 
Co-Authored-By: Juan José <soyjuanarbol@gmail.com>
juanarbol requested changes Apr 3, 2020
View reviewed changes
Comment thread test/parallel/test-async-wrap-constructor.js Outdated
fix message in test/parallel/test-async-wrap-constructor.js
0dbe2cf 
Co-Authored-By: Juan José <soyjuanarbol@gmail.com>

dericop commented Apr 5, 2020

Copy link
Copy Markdown
Contributor Author

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

addaleax commented Apr 5, 2020

Copy link
Copy Markdown
Member

@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 commented Apr 13, 2020

Copy link
Copy Markdown
Member

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 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 23, 2020

nodejs-github-bot commented Apr 23, 2020

Copy link
Copy Markdown
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
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 commented Apr 24, 2020

Copy link
Copy Markdown
Member

Landed in e99ec39 🎉

Thank you!! Amazing

juanarbol closed this Apr 24, 2020
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
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 mentioned this pull request Apr 27, 2020
Merged
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
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
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 mentioned this pull request Apr 28, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
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
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 mentioned this pull request May 2, 2020
Merged
targos pushed a commit that referenced this pull request May 13, 2020
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

himself65 himself65 left review comments
addaleax addaleax approved these changes
juanarbol juanarbol approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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

Back | FazBrowse Home | New Git URL