Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

[Converge] child_process argument type checking #22

Closed
jasnell opened this issue May 21, 2015 · 6 comments
Closed

[Converge] child_process argument type checking #22

jasnell opened this issue May 21, 2015 · 6 comments
Assignees

Comments

@jasnell
Copy link
Member

jasnell commented May 21, 2015

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

@sam-github
Copy link
Contributor

@piscisaureus I think these are the commits you said you didn't bring over. My primary involvement in this was to make the code actually match the docs in terms of what args were or were not optional. But that fell into a can of worms because some vocal users wanted very specific type checking on the args. Which isn't a bad thing, but with four args, most optional, it gets a bit hairy. I leave it to you all to decide what to do about this.

And note that the originally claimed "API incompatiblity" was a LACK of a throw...

@sam-github
Copy link
Contributor

I think at least the tests should be ported over, since they demonstrate the very wide variety of calling conventions that are possible... what the correct behaviour should be for each one can then be discussed more readily.

@jasnell
Copy link
Member Author

jasnell commented May 22, 2015

@nodejs/tsc ... this may be one we need broader review on.

@bnoordhuis
Copy link
Member

The changes look reasonable to me. If one of you turns this into a PR and cc's me, I'll sign off on it.

@jasnell
Copy link
Member Author

jasnell commented May 27, 2015

I'll be working on doing so

@rvagg
Copy link
Member

rvagg commented Aug 24, 2015

now active @ nodejs/node#2515

@rvagg rvagg closed this as completed Aug 24, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants