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

Fix #30068: refactored to use arrow functions #30069

Closed
wants to merge 1 commit into from
Closed

Fix #30068: refactored to use arrow functions #30069

wants to merge 1 commit into from

Conversation

mpark86
Copy link
Contributor

@mpark86 mpark86 commented Oct 22, 2019

Modified common.mustCall() to use arrow functions.

@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Oct 22, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 22, 2019 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/26152/ ✔️

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 22, 2019
@gireeshpunathil
Copy link
Member

@mpark86 - the build failure is due to the error here:
https://travis-ci.com/nodejs/node/jobs/248453662#L259
Invalid subsystem: "Fix #30068" - the commit message should follow a guideline (the message should start with a valid subsystem, followed by a colon, followed by a space and then followed by a short description, altogether less than 52 characters)

your commit message can be:
test: refactor to use arrow functions

@mpark86
Copy link
Contributor Author

mpark86 commented Oct 24, 2019

@gireeshpunathil Oh okay. Thanks for letting me know. How would you recommend to do it? Is there a way to change the commit message for pushed changes?

@gireeshpunathil
Copy link
Member

@mpark86 - here are the steps:

  • git rebase -i
  • this will open up an editor for modifying your commit // default editor is vi I guess
  • you should see one line at the top as below, everything else should be comments
  • pick fd0b250 Fix #30068: refactored to use arrow functions
  • change pick to reword // make sure the space in between is preserved
  • save and quit // I usually do :wq
  • the editor opens up again, this time with the commit message opened up for editing
  • change it to test: refactor to use arrow functions
  • save and quit again
  • issue git push -f origin issue-30068

Trott pushed a commit that referenced this pull request Oct 25, 2019
@mpark86 @Trott
test: use arrow functions for callbacks
175c318 
Use arrow functions for callbacks in
test/addons/make-callback-recurse/test.js.

PR-URL: #30069
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 25, 2019

Landed in 175c318

@Trott Trott closed this Oct 25, 2019
@Trott
Copy link
Member

Trott commented Oct 25, 2019

Thanks for the contribution! 🎉

targos pushed a commit that referenced this pull request Oct 28, 2019
@mpark86 @targos
test: use arrow functions for callbacks
1d9f427 
Use arrow functions for callbacks in
test/addons/make-callback-recurse/test.js.

PR-URL: #30069
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@mpark86
Copy link
Contributor Author

mpark86 commented Oct 28, 2019

Thank you. It was good to contribute to Node.js project!

@targos targos mentioned this pull request Nov 5, 2019
Merged
targos pushed a commit that referenced this pull request Nov 8, 2019
@mpark86 @targos
test: use arrow functions for callbacks
5191f0f 
Use arrow functions for callbacks in
test/addons/make-callback-recurse/test.js.

PR-URL: #30069
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
@mpark86 @targos
test: use arrow functions for callbacks
4a740d5 
Use arrow functions for callbacks in
test/addons/make-callback-recurse/test.js.

PR-URL: #30069
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request Nov 10, 2019
@mpark86 @targos
test: use arrow functions for callbacks
bc03fb3 
Use arrow functions for callbacks in
test/addons/make-callback-recurse/test.js.

PR-URL: #30069
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Nov 10, 2019
Merged
targos pushed a commit that referenced this pull request Nov 11, 2019
@mpark86 @targos
test: use arrow functions for callbacks
d96e8b6 
Use arrow functions for callbacks in
test/addons/make-callback-recurse/test.js.

PR-URL: #30069
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cretezy Cretezy Cretezy approved these changes

@Trott Trott Trott approved these changes

@trivikr trivikr trivikr approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@benjamingr benjamingr benjamingr approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. 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.
8 participants
@mpark86 @nodejs-github-bot @gireeshpunathil @Trott @addaleax @benjamingr @Cretezy @trivikr