child_process: remove unused argument#37923
Conversation
nodejs-github-bot
added
child_process
RaisinTen
added
the
author ready
|
nit: removed -> remove in the commit message |
Trott
changed the title
| function spawn(file, args, options) { | ||
| options = normalizeSpawnArguments(file, args, options); | ||
| validateTimeout(options.timeout, 'options.timeout'); | ||
| validateTimeout(options.timeout); |
There was a problem hiding this comment.
Hmm.. should we alternatively modify the validator function to use the name?
There was a problem hiding this comment.
I think that is a good idea.
There was a problem hiding this comment.
This is a validator function defined and used only in this file. If we add that argument, it would always be the same string literal, 'options.timeout'. It never validates anything that isn't options.timeout. I'd prefer to leave it alone unless/until that becomes a feature it needs.
I will note that it currently reports this as the error message:
The value of "timeout" is out of range. It must be an unsigned integer.
I think that's not at all a problem as the user will know they sent a "timeout" property (and may not always know what "options" refers to without looking at the docs). But if we want to update that to say "options.timeout", let's just update it inside validateTimeout() and not add a basically unused second argument. YAGNI and all that.
|
Landed in db7df59 |
The internal validateTimeout() takes a single parameter, so do not pass a second value. PR-URL: #37923 Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
The internal validateTimeout() takes a single parameter, so do not pass a second value. PR-URL: #37923 Reviewed-By: Zijian Liu <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
8 participants
The internal validateTimeout() takes a single parameter, so do not pass
a second value.