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

Change callback to ES6 style #24513

Closed
wants to merge 1 commit into from
Closed

Change callback to ES6 style #24513

wants to merge 1 commit into from

Conversation

jamesgeorge007
Copy link
Contributor

@jamesgeorge007 jamesgeorge007 commented Nov 20, 2018 •
edited
Loading

Converted the es5 functions present within the test/pummel/test-net-pause.js to arrow (es6) functions which is more concise.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 20, 2018
@gireeshpunathil gireeshpunathil added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 21, 2018
Trott
Trott previously requested changes Nov 21, 2018
View reviewed changes
test/pummel/test-net-pause.js Outdated Show resolved Hide resolved
@jamesgeorge007
Copy link
Contributor Author

@Trott What do you think now?

@gireeshpunathil
Copy link
Member

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

@gireeshpunathil
Copy link
Member

earlier CI run was aborted. new CI: https://ci.nodejs.org/job/node-test-pull-request/18879/

@addaleax
Copy link
Member

@jamesgeorge007 If you could rebase out the merge commit here, that could help – our CI doesn’t play well with those.

CI (rebasing disabled): https://ci.nodejs.org/job/node-test-pull-request/18890/

@jamesgeorge007
Copy link
Contributor Author

jamesgeorge007 commented Nov 23, 2018 •
edited
Loading

@addaleax My working tree became dirty. Hence, I had to close the PR and reopen it again.
Hope everythings fine now 👍

@addaleax
Copy link
Member

@jamesgeorge007 Hm … I think the linter failure from https://travis-ci.com/nodejs/node/jobs/160538871 is real:

$ make lint
make[1]: Entering directory `/home/travis/build/nodejs/node'
Running JS linter...
/home/travis/build/nodejs/node/test/pummel/test-net-pause.js
  91:4  error  Newline required at end of file but not found  eol-last
✖ 1 problem (1 error, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

@addaleax
Copy link
Member

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

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 24, 2018
@jamesgeorge007 jamesgeorge007 force-pushed the patch-1 branch 2 times, most recently from 2947c85 to d32d64c Compare November 24, 2018 03:07
@Trott
Copy link
Member

Trott commented Nov 24, 2018

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

@gireeshpunathil
Copy link
Member

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

@jamesgeorge007 jamesgeorge007 force-pushed the patch-1 branch 3 times, most recently from 17a9ef5 to b62fe77 Compare November 24, 2018 16:53
@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 25, 2018
@jamesgeorge007 jamesgeorge007 force-pushed the patch-1 branch 3 times, most recently from cdb949f to dee06f8 Compare November 25, 2018 09:35
@jamesgeorge007
test: convert callback to arrow function
b210dc2 
add newline at EOF 👍

Add EOF 👍

fix 👍

fix 👍

fix 👍
@jamesgeorge007 jamesgeorge007 changed the title Convert functions to es6 style Change callback to ES6 style Nov 25, 2018
@gireeshpunathil
Copy link
Member

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

@gireeshpunathil
Copy link
Member

landed as afab340

thank you @jamesgeorge007 for the contribution! Wish you great success with continued contribution to this project, if you are further interested please have a look at https://www.nodetodo.org/next-steps

gireeshpunathil pushed a commit that referenced this pull request Nov 25, 2018
@jamesgeorge007 @gireeshpunathil
test: convert callback to arrow function
afab340 
PR-URL: #24513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@jamesgeorge007
Copy link
Contributor Author

👍

@gireeshpunathil
Copy link
Member

@gireeshpunathil What was the reason behind closing this PR instead of merging? Just curious

@jamesgeorge007 - don't worry; your code is landed as afab340 into the repo, and you have become a contributor in the project.

@refack refack added the landed label Nov 25, 2018
targos pushed a commit that referenced this pull request Nov 27, 2018
@jamesgeorge007 @targos
test: convert callback to arrow function
e7b5187 
PR-URL: #24513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
rvagg pushed a commit that referenced this pull request Nov 28, 2018
@jamesgeorge007 @rvagg
test: convert callback to arrow function
a82b420 
PR-URL: #24513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
Merged
4 tasks
This was referenced Dec 7, 2018
Closed
Closed
Closed
Closed
Closed
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@jamesgeorge007 @refack
test: convert callback to arrow function
7601157 
PR-URL: nodejs#24513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 11, 2019
@jamesgeorge007 @BethGriggs
test: convert callback to arrow function
372073e 
PR-URL: #24513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@jamesgeorge007 @rvagg
test: convert callback to arrow function
c779ca0 
PR-URL: #24513
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.