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

[Converge] child_process argument type checking #2515

Closed
rvagg opened this issue Aug 24, 2015 · 9 comments
Closed

[Converge] child_process argument type checking #2515

rvagg opened this issue Aug 24, 2015 · 9 comments
Assignees
@trevnorris
Labels
child_process Issues and PRs related to the child_process subsystem.
Milestone
4.0.0

Comments

@rvagg
Copy link
Member

rvagg commented Aug 24, 2015

Continuing from nodejs/node-convergence-archive#22, I don't believe this has been done yet, @jasnell can you confirm please? That thread seems to have enough agreement to pull these changes in. Marking on the 4.0.0 milestone.

These commits add argument type checking to methods within child_process. This needs to be reconciled with the current io.js behavior. The change introduces a new throw so there's a potential API compatibility issue.
/cc @trevnorris
@rvagg rvagg added this to the 4.0.0 milestone Aug 24, 2015
@rvagg rvagg mentioned this issue Aug 24, 2015
Closed
@ChALkeR ChALkeR added the child_process Issues and PRs related to the child_process subsystem. label Aug 24, 2015
@rvagg rvagg mentioned this issue Aug 24, 2015
Closed
10 tasks
@jasnell
Copy link
Member

jasnell commented Aug 24, 2015

Correct, AFAIK this has not yet been completed. It's actually on my list for later this week but if someone gets to it before then ..... ;-)

@jasnell
Copy link
Member

jasnell commented Aug 24, 2015

@sam-github @trevnorris ... this particular set of commits were yours originally, I believe. Would either of you be able to take a look to see what needs to land in nodejs/node?

@trevnorris
Copy link
Contributor

lint fix doesn't matter. processing the arguments the same would be helpful, but not sure if it's currently done that way ATM. so may be worth bringing in.

@rvagg
Copy link
Member Author

rvagg commented Aug 25, 2015

@trevnorris I'm assigning this to you, please close this when it lands, before the end of the week please.

@trevnorris
Copy link
Contributor

@rvagg want me to just cherry-pick the applicable onto master?

@rvagg
Copy link
Member Author

rvagg commented Aug 25, 2015

@trevnorris your call, including assessing whether this is even necessary, PR obviously even if cherry-picking.

@rvagg
Copy link
Member Author

rvagg commented Aug 30, 2015

ping @trevnorris, are we close here?

@jasnell
Copy link
Member

jasnell commented Sep 2, 2015

@rvagg @trevnorris ... I'm going to take a look at this today. I'll port over the changes and open a new PR against master

@jasnell jasnell mentioned this issue Sep 2, 2015
Closed
jasnell added a commit that referenced this issue Sep 3, 2015
@jasnell
child_process: check execFile and fork args
107cbd6 
Port of joyent/node commits:

 * nodejs/node-v0.x-archive@e17c5a7
 * nodejs/node-v0.x-archive@70dafa7

Pull over test-child-process-spawn-typeerror.js from v0.12, replacing
the existing test in master. The new test includes a broader set of
tests on the various arg choices and throws.

Reviewed-By: trevnorris - Trevor Norris <trevnorris@nodejs.org>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani
PR-URL: #2667
Fixes: #2515
jasnell added a commit that referenced this issue Sep 3, 2015
@jasnell @Fishrock123
child_process: check execFile and fork args
4ea22a1 
Port of joyent/node commits:

 * nodejs/node-v0.x-archive@e17c5a7
 * nodejs/node-v0.x-archive@70dafa7

Pull over test-child-process-spawn-typeerror.js from v0.12, replacing
the existing test in master. The new test includes a broader set of
tests on the various arg choices and throws.

Reviewed-By: trevnorris - Trevor Norris <trevnorris@nodejs.org>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani
PR-URL: #2667
Fixes: #2515
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Sep 3, 2015
@jasnell @Fishrock123
child_process: check execFile and fork args
92c6512 
Port of joyent/node commits:

 * nodejs/node-v0.x-archive@e17c5a7
 * nodejs/node-v0.x-archive@70dafa7

Pull over test-child-process-spawn-typeerror.js from v0.12, replacing
the existing test in master. The new test includes a broader set of
tests on the various arg choices and throws.

Reviewed-By: trevnorris - Trevor Norris <trevnorris@nodejs.org>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani
PR-URL: nodejs#2667
Fixes: nodejs#2515
@rvagg
Copy link
Member Author

rvagg commented Sep 5, 2015

closed by #2667 I believe

@rvagg rvagg closed this as completed Sep 5, 2015
jasnell added a commit that referenced this issue Sep 6, 2015
@jasnell @rvagg
child_process: check execFile and fork args
7be4e49 
Port of joyent/node commits:

 * nodejs/node-v0.x-archive@e17c5a7
 * nodejs/node-v0.x-archive@70dafa7

Pull over test-child-process-spawn-typeerror.js from v0.12, replacing
the existing test in master. The new test includes a broader set of
tests on the various arg choices and throws.

Reviewed-By: trevnorris - Trevor Norris <trevnorris@nodejs.org>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani
PR-URL: #2667
Fixes: #2515
jasnell added a commit that referenced this issue Sep 6, 2015
@jasnell @rvagg
child_process: check execFile and fork args
609db5a 
Port of joyent/node commits:

 * nodejs/node-v0.x-archive@e17c5a7
 * nodejs/node-v0.x-archive@70dafa7

Pull over test-child-process-spawn-typeerror.js from v0.12, replacing
the existing test in master. The new test includes a broader set of
tests on the various arg choices and throws.

Reviewed-By: trevnorris - Trevor Norris <trevnorris@nodejs.org>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani
PR-URL: #2667
Fixes: #2515
jasnell added a commit that referenced this issue Sep 6, 2015
@jasnell @rvagg
child_process: check execFile and fork args
02bc29d 
Port of joyent/node commits:

 * nodejs/node-v0.x-archive@e17c5a7
 * nodejs/node-v0.x-archive@70dafa7

Pull over test-child-process-spawn-typeerror.js from v0.12, replacing
the existing test in master. The new test includes a broader set of
tests on the various arg choices and throws.

Reviewed-By: trevnorris - Trevor Norris <trevnorris@nodejs.org>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani
PR-URL: #2667
Fixes: #2515
jasnell added a commit that referenced this issue Sep 6, 2015
@jasnell @rvagg
child_process: check execFile and fork args
32037b7 
Port of joyent/node commits:

 * nodejs/node-v0.x-archive@e17c5a7
 * nodejs/node-v0.x-archive@70dafa7

Pull over test-child-process-spawn-typeerror.js from v0.12, replacing
the existing test in master. The new test includes a broader set of
tests on the various arg choices and throws.

Reviewed-By: trevnorris - Trevor Norris <trevnorris@nodejs.org>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: thefourtheye - Sakthipriyan Vairamani
PR-URL: #2667
Fixes: #2515
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@trevnorris trevnorris

Labels
child_process Issues and PRs related to the child_process subsystem.
Projects
None yet
Milestone
4.0.0
Development

No branches or pull requests
4 participants
@trevnorris @ChALkeR @jasnell @rvagg