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: child_process documentation for stdio #9637

Closed
wants to merge 1 commit into from

Conversation

skovhus
Copy link
Contributor

@skovhus skovhus commented Nov 16, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

Document that execFileSync, execSync and spawnSync also supports stdio as an Array.

execFileSync, execSync and spawnSync all ends up here, where the options are accepted as string or array. See https://github.com/nodejs/node/blob/master/lib/internal/child_process.js#L758

Fixes: #9636

@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 16, 2016
@Fishrock123
Copy link
Contributor

Fishrock123 commented Nov 16, 2016

Your git author information is skovhus <kenneth.skovhus@gmail.com>, are you ok with that or would you like to set more of your name than skovhus?

You can do so via:

git config --global user.name "J. Random User"

Then

git commit --amend --reset-author
git push origin head --force

@skovhus
Copy link
Contributor Author

skovhus commented Nov 16, 2016

@Fishrock123 I've updated with full name... Thanks 👍

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

You commit message should describe the changes, it doesn't now.

I suggest something like:

doc: child_process .stdio accepts an Array type

@skovhus
Copy link
Contributor Author

skovhus commented Nov 16, 2016

@sam-github thanks, I've updated the commit message.

@sam-github
Copy link
Contributor

@skovhus I'm sorry, I typed too quickly, and you accepted my suggestion too quickly.... it's the String type you added to the docs, Array was pre-existing.

Document that `execFileSync`, `execSync` and `spawnSync` also supports `stdio` as an Array.

Fixes: nodejs#9636
@skovhus
Copy link
Contributor Author

skovhus commented Nov 16, 2016

@sam-github my bad. Should be all right now.

@silverwind
Copy link
Contributor

Thanks! Landed in 6df167c.

@silverwind silverwind closed this Nov 18, 2016
silverwind pushed a commit that referenced this pull request Nov 18, 2016
Document that `execFileSync`, `execSync` and `spawnSync` also support
`stdio` as an Array.

PR-URL: #9637
Fixes: #9636
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <sam@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@skovhus
Copy link
Contributor Author

skovhus commented Nov 19, 2016

Thanks. When are the docs released? Would that be with the next release of node or do you update existing documentation?

@skovhus
Copy link
Contributor Author

skovhus commented Nov 19, 2016

@silverwind

@addaleax
Copy link
Member

do you update existing documentation?

The docs under https://nodejs.org/docs/ always refer to released versions and are fixed for a specific released version.

Would that be with the next release of node

Generally, yes – but that’s only scheduled 3 days from now. It would be awesome if you could check for which release lines (v4.x, v6.x, v7.x) this updated documentation applies so that we know to which of these it should be applied, too.

@skovhus skovhus deleted the docs-childprocess branch November 19, 2016 22:20
@skovhus
Copy link
Contributor Author

skovhus commented Nov 19, 2016

@addaleax thanks for clarifying.

It would be awesome if you could check for which release lines (v4.x, v6.x, v7.x) this updated documentation applies so that we know to which of these it should be applied, too.

I just checked that this documentation update applies for v4-7. So should I do a pull request where this is cherry picked into the v4.x and v5.x branch?

@addaleax
Copy link
Member

@skovhus Since this doesn’t apply cleanly to v4.x, I think that would be appreciated; you’ll want to target the v4.x-staging branch for that.

v5.x is no longer maintained and this cherry-picks cleanly onto the v6.x and v7.x staging branches, so there’s nothing to do there. :)

@skovhus
Copy link
Contributor Author

skovhus commented Nov 19, 2016

@addaleax ok, I've made a pull for updating v4.x-staging. : )

@addaleax
Copy link
Member

@skovhus thanks! :)

MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
Document that `execFileSync`, `execSync` and `spawnSync` also support
`stdio` as an Array.

PR-URL: #9637
Fixes: #9636
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <sam@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Nov 22, 2016
Document that `execFileSync`, `execSync` and `spawnSync` also support
`stdio` as an Array.

PR-URL: #9637
Fixes: #9636
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <sam@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
addaleax pushed a commit that referenced this pull request Nov 22, 2016
Document that `execFileSync`, `execSync` and `spawnSync` also support
`stdio` as an Array.

PR-URL: #9637
Fixes: #9636
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <sam@strongloop.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
facebook-github-bot pushed a commit to facebook/flow that referenced this pull request Dec 2, 2016
Summary:
This fixes #2824

I would wait merging it before nodejs/node#9637 has been approved.
Closes #2825

Differential Revision: D4268009

Pulled By: gabelevi

fbshipit-source-id: 1493ee7393516792fc08581cf0c71bc719c85f57
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants