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.

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

doc: update child process document #40700

Closed
wants to merge 1 commit into from
Closed

doc: update child process document #40700

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 2, 2021

update the description of stream in options.stdio.

Fixes: #15714

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Nov 2, 2021
Comment on lines 834 to 837
occurred). Because the stream is already piped (data is piped from the
stdio to the stream), thus stopping its readable side, otherwise we
might read data from the stream when at the same time the child
process does. If you want to pass a handle to child process, you can
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the sentence starting with "Because" is saying. Is anyone else having the same problem? Does it need to be re-worded a little bit?

Copy link
Author

@ghost ghost Nov 4, 2021

Choose a reason for hiding this comment

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

thank you very much! I'll check around

Copy link
Author

Choose a reason for hiding this comment

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

"Because the stream is already piped (data is piped from the stdio to the stream)" replace by "Because the stream is already piped to stdio". Would this be better?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's a bit more clear but it's still not a complete sentence and mildly puzzling. Maybe it's the "Because" that's throwing me off? Does this seem correct to you?

Suggested change
occurred). Because the stream is already piped (data is piped from the
stdio to the stream), thus stopping its readable side, otherwise we
might read data from the stream when at the same time the child
process does. If you want to pass a handle to child process, you can
occurred). The stream is already piped to stdio, thus stopping its readable
side. Otherwise we might read data from the stream at the same time that the
child process does. If you want to pass a handle to child process, you can

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for you help! You consideration is correct, I didn't describe it clearly. Or how about this?
"we need to stop its readable side, because the stream is already piped to the stdio."

Copy link
Member

Choose a reason for hiding this comment

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

I fell like there's a subtly I might be missing here. Maybe someone from @nodejs/child_process can take a look and confirm or suggest language?

Copy link
Member

Choose a reason for hiding this comment

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

I fell like there's a subtly I might be missing here. Maybe someone from @nodejs/child_process can take a look and confirm or suggest language?

Maybe someone from @nodejs/documentation?

@Trott
Copy link
Member

Trott commented Nov 3, 2021

Welcome, @w2k-lab, and thanks for the pull request. We'll want to make the first line of the commit message more descriptive. Maybe this?

doc: improve stream in child_process stdio option

Update the description of stream in options.stdio.

@Ayase-252 Ayase-252 added the review wanted PRs that need reviews. label Nov 17, 2021
update the description of stream in options.stdio.

Fixes: #15714
@marsonya marsonya requested a review from Trott March 2, 2022 11:10
@Trott
Copy link
Member

Trott commented Mar 2, 2022

@marsonya marsonya requested a review from Trott 13 hours ago

I'm sorry. I honestly don't understand what the added text is trying to communicate. I think "stop its readable" is supposed to be "stops it readable" and that "avoid read from" is supposed to be "avoid reading from". But it's also possible that I am not understanding what is being communicated and the current proposed text is right.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. review wanted PRs that need reviews.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handing spawn() stdio as socket wont duplex properly
4 participants