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, child_process: migrate to using internal/errors #11300

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
5 participants
@jasnell
Member

jasnell commented Feb 10, 2017

Ref: #11273

Semver-major because error messages are changed.

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)

errors, child_process

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 22, 2017

Member

@nodejs/ctc ... can I please get a review on this?

Member

jasnell commented Mar 22, 2017

@nodejs/ctc ... can I please get a review on this?

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Mar 22, 2017

@jasnell jasnell added the ctc-review label Mar 22, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Mar 22, 2017

@targos

almost there

Show outdated Hide outdated doc/api/errors.md
Show outdated Hide outdated doc/api/errors.md
Show outdated Hide outdated doc/api/errors.md
Show outdated Hide outdated doc/api/errors.md
Show outdated Hide outdated doc/api/errors.md
Show outdated Hide outdated lib/internal/errors.js
Show outdated Hide outdated lib/internal/child_process.js
Show outdated Hide outdated lib/internal/child_process.js
@mhdawson

Generally looks ok to me, but needs updates requested by @targos

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 20, 2017

Member

@targos @mhdawson ... this is unblocked now. PR is updated. PTAL

Member

jasnell commented Apr 20, 2017

@targos @mhdawson ... this is unblocked now. PR is updated. PTAL

@jasnell jasnell removed the blocked label Apr 20, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@@ -54,7 +61,6 @@ function message(key, args) {
// Utility function for registering the error codes. Only used here. Exported
// *only* to allow for testing.
function E(sym, val) {
assert(messages.has(sym) === false, `Error symbol: ${sym} was already used.`);

This comment has been minimized.

@targos

targos Apr 24, 2017

Member

Why did you remove this check? (The test for it is removed in the second commit btw)

@targos

targos Apr 24, 2017

Member

Why did you remove this check? (The test for it is removed in the second commit btw)

This comment has been minimized.

@jasnell

jasnell Apr 24, 2017

Member

Because of issues in the loading order of the errors.js module relative to assert.js on startup. There is a circular dependency that happens that causes the util.js used within assert.js to fail depending on when assert is loaded. I could replace this with a simple throw rather than the call to assert if you'd be more comfortable with that.

@jasnell

jasnell Apr 24, 2017

Member

Because of issues in the loading order of the errors.js module relative to assert.js on startup. There is a circular dependency that happens that causes the util.js used within assert.js to fail depending on when assert is loaded. I could replace this with a simple throw rather than the call to assert if you'd be more comfortable with that.

This comment has been minimized.

@targos

targos Apr 24, 2017

Member

I'm fine with removing it. I was just curious. The risk of reusing the same code is low if we keep them together and in alphabetical order.

@targos

targos Apr 24, 2017

Member

I'm fine with removing it. I was just curious. The risk of reusing the same code is low if we keep them together and in alphabetical order.

Show outdated Hide outdated lib/internal/errors.js
Show outdated Hide outdated lib/internal/errors.js
@@ -125,12 +125,6 @@ assert.throws(() => {
message: /^Error for testing 2/ }));
}, /AssertionError: .+ does not match \S/);
assert.doesNotThrow(() => errors.E('TEST_ERROR_USED_SYMBOL'));

This comment has been minimized.

@targos

targos Apr 24, 2017

Member

please bundle this change with the same commit that removed the error

@targos

targos Apr 24, 2017

Member

please bundle this change with the same commit that removed the error

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Apr 24, 2017

@targos @mhdawson ... PTAL

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Apr 24, 2017

@targos

targos approved these changes Apr 26, 2017 edited

LGTM. This needs a rebase.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 26, 2017

Member

Rebased. Ping @mhdawson

Member

jasnell commented Apr 26, 2017

Rebased. Ping @mhdawson

@mhdawson

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 27, 2017

Member

Thank you @mhdawson and @targos. Rebased again and new CI before landing: https://ci.nodejs.org/job/node-test-pull-request/7715/

Member

jasnell commented Apr 27, 2017

Thank you @mhdawson and @targos. Rebased again and new CI before landing: https://ci.nodejs.org/job/node-test-pull-request/7715/

jasnell added some commits Feb 10, 2017

errors: use lazy assert to avoid issues on startup
Use of assert must be lazy to allow errors to be used early
before the process is completely set up

jasnell added a commit that referenced this pull request Apr 27, 2017

errors: use lazy assert to avoid issues on startup
Use of assert must be lazy to allow errors to be used early
before the process is completely set up

PR-URL: #11300
Ref: #11273
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell added a commit that referenced this pull request Apr 27, 2017

errors, child_process: migrate to using internal/errors
PR-URL: #11300
Ref: #11273
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 27, 2017

Member

Landed in f0b7025 and 7632761

Member

jasnell commented Apr 27, 2017

Landed in f0b7025 and 7632761

@jasnell jasnell closed this Apr 27, 2017

@Trott Trott removed the ctc-review label May 1, 2017

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

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

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