FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

errors,http_server: migrate to use internal/errors.js by bidipyne · Pull Request #13301 · nodejs/node · GitHub

/ node Public

errors,http_server: migrate to use internal/errors.js - #13301

Closed
bidipyne wants to merge 1 commit into
nodejs:masterfrom
bidipyne:my-repo
ConversationCommits (1)ChecksFiles changed
Closed

errors,http_server: migrate to use internal/errors.js#13301
bidipyne wants to merge 1 commit into
nodejs:masterfrom
bidipyne:my-repo

Conversation

bidipyne commented May 30, 2017

Copy link
Copy Markdown
Contributor

Checklist

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

_http_server.js
internal/errors.js

ref: #11273
I read and understood the contribution guidelines, please review and suggest.
@jasnell
Thanks.

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 May 30, 2017
jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 30, 2017
jasnell reviewed May 30, 2017
View reviewed changes
Comment thread lib/_http_server.js Outdated

jasnell May 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since we're updating anyway, it would be nice to get rid of the contraction.

Cannot render headers ...

jasnell May 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ERR_ASSERTION is the wrong choice here. That should be reserved for uses of the assert module. A new error code would be more appropriate.

bidipyne May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@jasnell Rectifications done. Thank You

jasnell reviewed May 30, 2017
View reviewed changes
Comment thread lib/_http_server.js Outdated

jasnell May 30, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Likewise, ERR_ASSERTION is the wrong choice here. A new error code would be better.

bidipyne May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done!

jasnell reviewed May 31, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated

jasnell May 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since this is specific to http, perhaps ERR_HTTP_INVALID_CHAR

bidipyne May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Re-phrased.

jasnell reviewed May 31, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated

jasnell May 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

same here... ERR_HTTP_INVALID_STATUS_CODE

bidipyne May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done!

jasnell reviewed May 31, 2017
View reviewed changes
Comment thread lib/internal/errors.js Outdated

jasnell May 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ERR_HTTP_HEADERS_SENT ?

bidipyne May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Rectified. Thanks!

jasnell reviewed May 31, 2017
View reviewed changes
Comment thread test/parallel/test-http-response-statuscode.js Outdated

jasnell May 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This shouldn't need the --expose-internals flag

bidipyne May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Removed.

jasnell reviewed May 31, 2017
View reviewed changes
Comment thread test/parallel/test-http-write-head.js Outdated

jasnell May 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Likewise, this shouldn't need the --expose-internals flag

bidipyne May 31, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Removed.

mhdawson approved these changes May 31, 2017
View reviewed changes

mhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with a nit

Comment thread lib/_http_server.js Outdated

mhdawson May 31, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm guessing its broken into 2 strings to avoid the long line. I'm not sure I've seen us use the + for this, instead could you just put the 'Invalid ...' on a second line

bidipyne Jun 1, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done as per suggestion. Thanks!

jasnell commented Jun 1, 2017

Copy link
Copy Markdown
Member

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

jasnell pushed a commit that referenced this pull request Jun 2, 2017
errors,http_server: migrate to use internal/errors.js
a9f798e 
PR-URL: #13301
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Jun 2, 2017

Copy link
Copy Markdown
Member

Landed in a9f798e

jasnell closed this Jun 2, 2017
lance mentioned this pull request Oct 25, 2017
Closed
2 tasks
jasnell mentioned this pull request Oct 30, 2017
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+1 more reviewer
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL