Skip to content

child_process: use internal/errors - #14009

Closed
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:child-proc
Closed

child_process: use internal/errors#14009
tniessen wants to merge 2 commits into
nodejs:masterfrom
tniessen:child-proc

Conversation

@tniessen

@tniessentniessen commented Jun 30, 2017•
edited
Loading

Copy link
Copy Markdown
Member

Refs: #11273

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process

@nodejs-github-botnodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jun 30, 2017
@tniessen

tniessen commented Jun 30, 2017

Copy link
Copy Markdown
MemberAuthor

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

@tniessentniessen added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jun 30, 2017
@tniessentniessen self-assigned this Jun 30, 2017
@jasnelljasnell mentioned this pull request Jun 30, 2017
Closed
80 tasks
@TrottTrott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 30, 2017
lpinca
lpinca reviewed Jul 1, 2017
View reviewed changes
Comment threadlib/internal/child_process.js Outdated
} else if (options !== undefined &&
(options === null || typeof options !== 'object')) {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'Object');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'options', 'Object',

@lpincalpincaJul 1, 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.

Why is 'Object' capitalized here?

@tniessentniessenJul 1, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It was like that before, should I change it?

@lpincalpincaJul 1, 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.

Yeah I noticed but few lines above (260) it is in lowercase. I'd make them consistent.

@tniessen

tniessen commented Jul 2, 2017

Copy link
Copy Markdown
MemberAuthor

@nodejs/ctc due to semverity

mcollina
mcollina approved these changes Jul 2, 2017
View reviewed changes

@mcollinamcollina 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.

LGTM as semver-major

tniessen added a commit that referenced this pull request Jul 3, 2017
@tniessen
child_process: use internal/errors
fe730d3
PR-URL: #14009
Refs: #11273
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@tniessen

tniessen commented Jul 3, 2017

Copy link
Copy Markdown
MemberAuthor

Landed in fe730d3. CI on master: https://ci.nodejs.org/job/node-test-commit-linuxone/7044/

@tniessentniessen closed this Jul 3, 2017
@jasnelljasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes
@targostargostargos approved these changes
+1 more reviewer
@refackrefackrefack approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tniessentniessen

Labels

child_processIssues and PRs related to the child_process subsystem.errorsIssues and PRs related to JavaScript errors originated in Node.js core.semver-majorPRs 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.

8 participants

@tniessen@mcollina@refack@jasnell@lpinca@targos@Trott@nodejs-github-bot