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

src: use custom TryCatch subclass #24751

Merged
merged 1 commit into from
Dec 3, 2018
Merged

src: use custom TryCatch subclass #24751

merged 1 commit into from
Dec 3, 2018

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Nov 30, 2018 •
edited
Loading

pulled out of #23926

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@devsnek build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1745/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 30, 2018
@devsnek devsnek added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Nov 30, 2018
@joyeecheung
Copy link
Member

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

@devsnek
Copy link
Member Author

devsnek commented Dec 3, 2018

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

@devsnek
src: use custom TryCatch subclass
f084e06 
PR-URL: nodejs#24751
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@devsnek
Copy link
Member Author

devsnek commented Dec 3, 2018

landed in f084e06

@devsnek devsnek merged commit f084e06 into nodejs:master Dec 3, 2018
@Trott
Copy link
Member

Trott commented Dec 3, 2018

This is getting-the-word-out and not a complaint/criticism, as the rule changed less than two months ago and not everyone has gotten the memo yet:

This should not have landed without a second review or waiting until it had been open 7 days. https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#code-reviews:

At least two Collaborators must approve a pull request before the pull request lands. (One Collaborator approval is enough if the pull request has been open for more than 7 days.)

@devsnek devsnek deleted the refactor/native-try-catch branch December 3, 2018 18:47
@devsnek
Copy link
Member Author

devsnek commented Dec 3, 2018

@Trott thanks, i'll remember that in the future 👍

@joyeecheung
Copy link
Member

Should be fixed by nodejs/node-core-utils#290 (I just updated the branch), anyone wants to take a look?

screen shot 2018-12-04 at 2 48 56 am

BridgeAR pushed a commit that referenced this pull request Dec 5, 2018
@devsnek @BridgeAR
src: use custom TryCatch subclass
c957adb 
PR-URL: #24751
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@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
@devsnek @refack
src: use custom TryCatch subclass
82c5176 
PR-URL: nodejs#24751
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BethGriggs
Copy link
Member

This change does not land cleanly on v10.x-staging. I've added the backport-requested-v10.x label, but feel free to swap to dont-land-on- if this change shouldn't land on v10.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devsnek @nodejs-github-bot @joyeecheung @Trott @BethGriggs