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.

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

_http_outgoing: migrate to use internal/errors #14735

Closed

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Aug 10, 2017

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

@refack
Copy link
Contributor

refack commented Aug 13, 2017

/cc @mcollina @mhdawson @fhinkel

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

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

Successfully merging this pull request may close these issues.

5 participants