Skip to content

child_process: refactor stdioStringToArray function - #27657

Closed
zero1five wants to merge 1 commit into
nodejs:masterfrom
zero1five:refactor/child-process
Closed

child_process: refactor stdioStringToArray function#27657
zero1five wants to merge 1 commit into
nodejs:masterfrom
zero1five:refactor/child-process

Conversation

@zero1five

@zero1fivezero1five commented May 12, 2019

Copy link
Copy Markdown
Contributor

reduce the function in both files to one.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label May 12, 2019
@zero1fivezero1five changed the title child_process: reactor stdioStringToArray functionchild_process: refactor stdioStringToArray functionMay 12, 2019
@zero1five
zero1fiveforce-pushed the refactor/child-process branch from fd4a945 to 5265c3dCompareMay 12, 2019 13:53

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

If I'm not mistaken this will change the internal stdio type in case of inherit to fd. This might not change anything because the fd itself set the same but I wanted to point this out.

@BridgeAR

Copy link
Copy Markdown
Member

@nodejs/child_process PTAL at #27657 (review)

@zero1five

Copy link
Copy Markdown
ContributorAuthor

@BridgeAR Ah... i see. It is indeed described in the documentation that you can use inherit to create ['inherit', 'inherit', 'inherit'] or [0, 1, 2], but I Think it would be better to use a single identifier when implementing it internal. Some?

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2019
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@danbev

danbev commented May 15, 2019

Copy link
Copy Markdown
Contributor

Re-build of failing node-test-commit-windows-fanned (✔️)

@zero1five
zero1fiveforce-pushed the refactor/child-process branch from 5265c3d to f78ff1eCompareMay 15, 2019 17:07
reduce the function in both files to one.
@zero1five
zero1fiveforce-pushed the refactor/child-process branch from f78ff1e to ccece4eCompareMay 15, 2019 17:24
@danbev

Copy link
Copy Markdown
Contributor

Landed in 9f99d4e.

@danbevdanbev closed this May 16, 2019
pullBot pushed a commit to shakir-abdo/node that referenced this pull request May 16, 2019
reduce the function in both files to one.
PR-URL: nodejs#27657
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@zero1five
zero1five deleted the refactor/child-process branch May 16, 2019 17:28
targos pushed a commit that referenced this pull request May 17, 2019
reduce the function in both files to one.
PR-URL: #27657
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeARBridgeAR mentioned this pull request May 21, 2019
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.child_processIssues and PRs related to the child_process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@zero1five@BridgeAR@nodejs-github-bot@danbev@jasnell@Trott