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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
6 participants
@sreepurnajasti
Contributor

sreepurnajasti commented May 29, 2017

  • 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

Show outdated Hide outdated lib/internal/errors.js Outdated
@jasnell

LGTM with a nit.

@mhdawson

LGTM

@mhdawson

This comment has been minimized.

Show comment
Hide comment
Member

mhdawson commented May 31, 2017

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 31, 2017

Member

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

Member

mhdawson commented May 31, 2017

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

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 31, 2017

Member

Actually 48 hours is already up landing.

Member

mhdawson commented May 31, 2017

Actually 48 hours is already up landing.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 31, 2017

Member

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?

Member

mhdawson commented May 31, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jun 2, 2017

Member

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

Member

jasnell commented Jun 2, 2017

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

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Jun 2, 2017

Member

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

Member

mhdawson commented Jun 2, 2017

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

@sreepurnajasti

This comment has been minimized.

Show comment
Hide comment
@sreepurnajasti

sreepurnajasti Jun 2, 2017

Contributor

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

Contributor

sreepurnajasti commented Jun 2, 2017

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Jun 2, 2017

@@ -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');

This comment has been minimized.

@addaleax

addaleax Jun 2, 2017

Member

There should be a space after installed., right?

@addaleax

addaleax Jun 2, 2017

Member

There should be a space after installed., right?

This comment has been minimized.

@jasnell

jasnell Jun 2, 2017

Member

good catch

@jasnell

jasnell Jun 2, 2017

Member

good catch

@addaleax

This comment has been minimized.

Show comment
Hide comment
Member

addaleax commented Jun 2, 2017

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Jun 5, 2017

Member

CI run good, landing. Will add space.

Member

mhdawson commented Jun 5, 2017

CI run good, landing. Will add space.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Jun 5, 2017

Member

Landed as 062071a

Member

mhdawson commented Jun 5, 2017

Landed as 062071a

@mhdawson mhdawson closed this Jun 5, 2017

mhdawson added a commit that referenced this pull request Jun 5, 2017

errors,process: migrate to use internal/errors.js
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 label Jun 5, 2017

@refack refack added this to Done in Error Codes Aug 20, 2017

@sreepurnajasti sreepurnajasti deleted the sreepurnajasti:my-branch branch Dec 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment