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

lib/child_process: convert to using internal/errors by ramimoshe · Pull Request #15090 · nodejs/node · GitHub

/ node Public

lib/child_process: convert to using internal/errors - #15090

Closed
ramimoshe wants to merge 2 commits into
nodejs:masterfrom
ramimoshe:master
ConversationCommits (2)ChecksFiles changed
Closed

lib/child_process: convert to using internal/errors#15090
ramimoshe wants to merge 2 commits into
nodejs:masterfrom
ramimoshe:master

Conversation

ramimoshe commented Aug 30, 2017

Copy link
Copy Markdown

covert lib/child_process.js over to using lib/internal/errors.js

ref: #11273

nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Aug 30, 2017
ramimoshe mentioned this pull request Aug 30, 2017
Closed
80 tasks
jasnell reviewed Aug 30, 2017
View reviewed changes
Comment thread lib/child_process.js Outdated
options = {};
else if (options === null || typeof options !== 'object')
throw new TypeError('"options" argument must be an object');
throw new errors.TypeError('ERR_OPTS_NOT_OBJECT');

jasnell Aug 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

The existing ERR_INVALID_ARG_TYPE would be better here.

jasnell Aug 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

(and else where throughout here)

jasnell requested changes Aug 30, 2017
View reviewed changes

jasnell 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

Thank you so much for doing this. I may be mistaken but I seem to recall another open PR that was looking at this file also. I'd recommend doing a quick search to make sure you're not accidentally duplicating effort. Either way, the work is deeply appreciated.

I left a couple of comments that should be addressed before this could proceed forward.

targos commented Aug 30, 2017

Copy link
Copy Markdown
Member

The other PR is here: #14998

gonenduk commented Aug 30, 2017

Copy link
Copy Markdown
Contributor

@targos - I believe the other PR #14998 is for the file under the internal folder.
This PR is for the one under lib.

targos commented Aug 30, 2017

Copy link
Copy Markdown
Member

They both change errors in lib/child_process.js

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Sep 19, 2017
jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 25, 2017

ramimoshe commented Sep 26, 2017

Copy link
Copy Markdown
Author

so can i close this issue?

BridgeAR commented Sep 28, 2017

Copy link
Copy Markdown
Member

Closing as this is a duplicate of another PR that is about to land.

BridgeAR closed this Sep 28, 2017

BridgeAR commented Sep 28, 2017

Copy link
Copy Markdown
Member

@ramimoshe thanks a lot for your work nevertheless! I am sorry that your PR could not land.

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 requested changes

Assignees

No one assigned

Labels

blocked PRs that are blocked by other issues or PRs. child_process Issues and PRs related to the child_process subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. 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

Back | FazBrowse Home | New Git URL