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

errors,process: migrate to use internal/errors.js #13285

Closed
wants to merge 1 commit into from
Closed

errors,process: migrate to use internal/errors.js #13285

wants to merge 1 commit into from

Conversation

sreepurnajasti
Copy link
Contributor

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

lib/internal/process.js
lib/internal/errors.js

ref: #11273

@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. labels May 29, 2017
jasnell
jasnell reviewed May 30, 2017
View reviewed changes
lib/internal/errors.js Outdated
@@ -151,6 +152,7 @@ E('ERR_SOCKET_BAD_TYPE',
E('ERR_SOCKET_CANNOT_SEND', 'Unable to send data');
E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536');
E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
E('ERR_V8BREAKITERATOR', 'full ICU data not installed. See https://github.com/nodejs/node/wiki/Intl');
Copy link
Member

Choose a reason for hiding this comment

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

long line here. please line wrap at <= 80 chars.
Running make lint should catch these :-)

Copy link
Contributor Author

@sreepurnajasti sreepurnajasti May 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@jasnell Unfortunately, on jslint test, this error is not showed up. Now, this is done. Thank you.

jasnell
jasnell approved these changes May 30, 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 a nit.

mhdawson
mhdawson approved these changes May 30, 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

@mhdawson
Copy link
Member

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

@mhdawson
Copy link
Member

mhdawson commented May 31, 2017 •
edited
Loading

CI run good, will land tomorrow once 48 hours has passed.

@mhdawson
Copy link
Member

Actually 48 hours is already up landing.

@mhdawson
Copy link
Member

mhdawson commented May 31, 2017 •
edited
Loading

Actually looking again as part of landing I think its better to avoid adding:

// Flags: --expose-internals

and just use TypeError instead of errors.TypeError etc when possible. @sreepurnajasti can you make that change?

@jasnell
Copy link
Member

jasnell commented Jun 2, 2017

This needs a rebase before it can land. Thank you!

@mhdawson
Copy link
Member

mhdawson commented Jun 2, 2017

@sreepurnajasti can you can rebase and remove the use of --expose-internals.

@sreepurnajasti
Copy link
Contributor Author

@jasnell @mhdawson I rebased. Also removed --expose-internals. Thank you

@jasnell
Copy link
Member

jasnell commented Jun 2, 2017

New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/8441/

addaleax
addaleax reviewed Jun 2, 2017
View reviewed changes
lib/internal/errors.js
@@ -160,6 +161,8 @@ E('ERR_SOCKET_BAD_TYPE',
E('ERR_SOCKET_CANNOT_SEND', 'Unable to send data');
E('ERR_SOCKET_BAD_PORT', 'Port should be > 0 and < 65536');
E('ERR_SOCKET_DGRAM_NOT_RUNNING', 'Not running');
E('ERR_V8BREAKITERATOR', 'full ICU data not installed.' +
'See https://github.com/nodejs/node/wiki/Intl');
Copy link
Member

Choose a reason for hiding this comment

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

There should be a space after installed., right?

Copy link
Member

Choose a reason for hiding this comment

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

good catch

@addaleax
Copy link
Member

addaleax commented Jun 2, 2017

CI: https://ci.nodejs.org/job/node-test-commit/10321/

@mhdawson
Copy link
Member

mhdawson commented Jun 5, 2017 •
edited
Loading

CI run good, landing. Will add space.

@mhdawson
Copy link
Member

mhdawson commented Jun 5, 2017

Landed as 062071a

@mhdawson mhdawson closed this Jun 5, 2017
mhdawson pushed a commit that referenced this pull request Jun 5, 2017
@sreepurnajasti @mhdawson
errors,process: migrate to use internal/errors.js
062071a 
PR-URL: #13285
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 5, 2017
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
@sreepurnajasti sreepurnajasti deleted the my-branch branch December 27, 2017 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil 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. process Issues and PRs related to the process 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.
6 participants
@sreepurnajasti @mhdawson @jasnell @addaleax @gireeshpunathil @nodejs-github-bot