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

_http_outgoing: migrate to use internal/errors #14735

Closed
wants to merge 1 commit into from
Closed

_http_outgoing: migrate to use internal/errors #14735

wants to merge 1 commit into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Aug 10, 2017 •
edited
Loading

Ref: #11273

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)

http, errors

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. labels Aug 10, 2017
jasnell
jasnell approved these changes Aug 10, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell 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 green CI

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 10, 2017
@refack refack self-assigned this Aug 12, 2017
@jasnell jasnell mentioned this pull request Aug 12, 2017
Closed
80 tasks
refack
refack approved these changes Aug 12, 2017
View reviewed changes
test/parallel/test-http-mutable-headers.js
common.expectsError(
() => res.setHeader(),
{
code: 'ERR_INVALID_HTTP_TOKEN',
Copy link
Contributor

Choose a reason for hiding this comment

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

@starkwang how much work will it to add the given value (in addition to the token name)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added some more information about the given value in the error message.

@starkwang starkwang force-pushed the http-outgoing-internal-errors branch from ae9c960 to 0fcece2 Compare August 13, 2017 15:29
@starkwang starkwang mentioned this pull request Aug 13, 2017
Closed
3 tasks
refack
refack approved these changes Aug 13, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯

@refack
Copy link
Contributor

refack commented Aug 13, 2017

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

@refack
Copy link
Contributor

refack commented Aug 13, 2017

/cc @mcollina @mhdawson @fhinkel

mhdawson
mhdawson approved these changes Aug 14, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks !

@mhdawson
Copy link
Member

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

@refack
Copy link
Contributor

refack commented Aug 14, 2017

Landed in 11a2ca2
(had a green CI from 23h ago)

@refack refack closed this Aug 14, 2017
refack pushed a commit that referenced this pull request Aug 14, 2017
@starkwang @refack
errors: migrate _http_outgoing
11a2ca2 
PR-URL: #14735
Refs: #11273
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@maclover7 maclover7 mentioned this pull request Nov 24, 2017
Closed
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@starkwang @refack @mhdawson @jasnell @nodejs-github-bot