Skip to content

Conversation

@jasnell
Copy link
Member

Finish migrating the zlib binding over to use internal/errors

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
Affected core subsystem(s)

zlib, errors

@jasnelljasnell added errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. zlib Issues and PRs related to the zlib subsystem. labels Oct 27, 2017
@nodejs-github-botnodejs-github-bot added 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. zlib Issues and PRs related to the zlib subsystem. labels Oct 27, 2017
@jasnell
Copy link
MemberAuthor

ping @nodejs/tsc

Copy link
Contributor

@refackrefack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW

Copy link
Member

@joyeecheungjoyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a question?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we need a goto here? (other than one less line of duplicate code?)

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just that... didn't want to duplicate the line of code

@joyeecheung
Copy link
Member

Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a new way to test this error once #16511 is merged.

@jasnelljasnellforce-pushed the migrate-zlib-internal-errors branch from 0257a1d to c8ef9c3CompareOctober 29, 2017 17:15
jasnell added a commit that referenced this pull request Oct 29, 2017
PR-URL: #16540 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@jasnell
Copy link
MemberAuthor

Landed in 896eaf6

@jasnelljasnell closed this Oct 29, 2017
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#16540 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Qard pushed a commit to ayojs/ayo that referenced this pull request Nov 2, 2017
PR-URL: nodejs/node#16540 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
PR-URL: nodejs/node#16540 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.errorsIssues and PRs related to JavaScript errors originated in Node.js core.semver-majorPRs that contain breaking changes and should be released in the next major version.zlibIssues and PRs related to the zlib subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@jasnell@joyeecheung@mcollina@refack@lpinca@XadillaX@nodejs-github-bot