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

child_process: align fork/spawn stdio error msg #11044

Closed
wants to merge 1 commit into from
Closed

child_process: align fork/spawn stdio error msg #11044

wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Jan 27, 2017 •
edited
Loading

fork()'s support for .stdio strings in 3268863 used a different
TypeError string from spawn, unnecessarily.

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Jan 27, 2017
@sam-github sam-github mentioned this pull request Jan 27, 2017
Closed
4 tasks
jasnell
jasnell previously approved these changes Jan 27, 2017
View reviewed changes
@sam-github
Copy link
Contributor Author

@mscdex approved?

Do I need to wait 2 days, or can I revert once there are two approvals?

cjihrig
cjihrig previously approved these changes Jan 27, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. However, if @mscdex's request to change the error message was the only thing outstanding, couldn't it just be updated here?

@mscdex
Copy link
Contributor

mscdex commented Jan 28, 2017

If nobody else had any questions/suggestions about the original changes, then it would probably be easier to just change the error message here instead of revert + change.

@sam-github
Copy link
Contributor Author

OK, I'll revise this PR to be an update of the error message. On monday, though. Cheers.

@sam-github sam-github changed the title Revert "child_process: add string shortcut for fork stdio" child_process: align fork/spawn stdio error msg Jan 30, 2017
@sam-github sam-github dismissed stale reviews from jasnell and cjihrig January 30, 2017 21:57

replaced revert with code, PTAL

@sam-github
Copy link
Contributor Author

@mscdex I fixed the error message, PTAL, sorry I didn't understand what you LGTM was conditional on.

mscdex
mscdex reviewed Jan 30, 2017
View reviewed changes
doc/api/child_process.md Outdated
@@ -299,6 +299,9 @@ output on this fd is expected to be line delimited JSON objects.
*Note: Unlike the fork(2) POSIX system call, `child_process.fork()` does
not clone the current process.*

*Note*: Support for non-Array values of the `.stdio` option was added in
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if we want to start adding this kind of information in this way or not. I think it's probably a separate discussion to be had outside of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't we usually put something in the API docs when we make breaking API changes? I'll check around to see what we've done in the recent past.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK we haven't been including version information when it comes to changes in behavior over time. That might be beneficial, but still it's a separate discussion on if we want to start doing that and if so, how something like that would be best conveyed to the end user (presumably not in a free-form way like this note).

sam-github
sam-github commented Jan 30, 2017
View reviewed changes
doc/api/child_process.md Outdated
@@ -299,6 +299,9 @@ output on this fd is expected to be line delimited JSON objects.
*Note: Unlike the fork(2) POSIX system call, `child_process.fork()` does
not clone the current process.*

*Note*: Support for non-Array values of the `.stdio` option was added in
Copy link
Contributor Author

@sam-github sam-github Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax (because you did the original YAML markup, I think) @MylesBorins Will this work?

Our 'ADDED' metadata is not granular enough. It tracks API additions, mostly, but it doesn't account for backporting though 00ffa33 gives me the impression it was intended to.

Since it doesn't track changes in APIs, will this note work? Will the FIXME (EDIT: <-- I meant REPLACEME) get processed?

Copy link
Member

Choose a reason for hiding this comment

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

Will the FIXME (EDIT: <-- I meant REPLACEME) get processed?

Yup, the releaser will be warned by the build script (which does just a plain “grep”) and replace it.

Our 'ADDED' metadata is not granular enough.

Working on that! ;)

mscdex
mscdex reviewed Jan 30, 2017
View reviewed changes
lib/child_process.js Outdated
case 'inherit':
return [option, option, option, 'ipc'];
}
throw new TypeError('Incorrect value of stdio option: ' + option);
Copy link
Contributor

@mscdex mscdex Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think this should instead go inside a default: case in the switch above.

@sam-github
child_process: align fork/spawn stdio error msg
f338521 
fork()'s support for .stdio strings in 3268863 used a different
TypeError string from spawn, unnecessarily.
@sam-github
Copy link
Contributor Author

PTAL @mscdex

@mscdex
Copy link
Contributor

mscdex commented Feb 1, 2017

LGTM if CI is ok with it: https://ci.nodejs.org/job/node-test-pull-request/6142/

@sam-github
Copy link
Contributor Author

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

One of the freebsd machines failed a number of cluster tests. Trying again, may be ephemeral.

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

The change LGTM but does this need to be semver-major? Are the error conditions changing here? It's not obvious

@jasnell
Copy link
Member

jasnell commented Feb 2, 2017

Scratch that, the error message change definitely makes this semver-major

@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. and removed dont-land-on-v7.x labels Feb 2, 2017
@sam-github
Copy link
Contributor Author

Yeah, its semver major because it builds on #10866, which was semver major. Unfortunately, because its really a feature that would be nice to get backported.

What was the word on adding new property values to options?

If that is NOT semver-major, then we can do a backportable version of #10866

@jasnell
Copy link
Member

jasnell commented Feb 10, 2017

Adding properties to options objects, so long as they are not required, is semver-minor.

jasnell pushed a commit that referenced this pull request Feb 11, 2017
@sam-github @jasnell
child_process: align fork/spawn stdio error msg
4cafa60 
fork()'s support for .stdio strings in 3268863 used a different
TypeError string from spawn, unnecessarily.

PR-URL: #11044
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell
Copy link
Member

jasnell commented Feb 11, 2017

Landed in 4cafa60

@jasnell jasnell closed this Feb 11, 2017
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
@sam-github @krydos
child_process: align fork/spawn stdio error msg
f6b7d62 
fork()'s support for .stdio strings in 3268863 used a different
TypeError string from spawn, unnecessarily.

PR-URL: nodejs#11044
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
@sam-github sam-github deleted the revert-10866 branch April 17, 2017 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@mscdex mscdex mscdex left review comments

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
child_process Issues and PRs related to the child_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
@sam-github @mscdex @jasnell @addaleax @cjihrig @nodejs-github-bot