Skip to content

Fix #30068: refactored to use arrow functions#30069

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
Closed
mpark86 wants to merge 1 commit into from
Closed

Fix #30068: refactored to use arrow functions #30069

mpark86 wants to merge 1 commit into from

Conversation

@mpark86
Copy link
Contributor

@mpark86mpark86 commented Oct 22, 2019

Modified common.mustCall() to use arrow functions.

@nodejs-github-botnodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Oct 22, 2019
Cretezy
Cretezy reviewed Oct 22, 2019
View reviewed changes
@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/ ✔️

@TrottTrott 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

gireeshpunathil commented Oct 23, 2019

@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
ContributorAuthor

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

gireeshpunathil commented Oct 24, 2019

@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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 25, 2019

Landed in 175c318

@TrottTrott 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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
@mpark86
Copy link
ContributorAuthor

mpark86 commented Oct 28, 2019

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

@targostargos 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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
@targostargos 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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@benjamingrbenjamingrbenjamingr approved these changes

@gireeshpunathilgireeshpunathilgireeshpunathil approved these changes

@trivikrtrivikrtrivikr approved these changes

+1 more reviewer

@CretezyCretezyCretezy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

addonsIssues and PRs related to native addons.author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues 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