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

child_process: remove unused argument #37923

Merged
merged 0 commits into from
Mar 28, 2021
Merged

child_process: remove unused argument #37923

merged 0 commits into from
Mar 28, 2021

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 26, 2021

The internal validateTimeout() takes a single parameter, so do not pass
a second value.

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Mar 26, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 26, 2021
@nodejs-github-bot
Copy link
Collaborator

@RaisinTen
Copy link
Contributor

nit: removed -> remove in the commit message

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott changed the title child_process: removed unused argument child_process: remove unused argument Mar 26, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@@ -600,7 +600,7 @@ function abortChildProcess(child, killSignal) {

function spawn(file, args, options) {
options = normalizeSpawnArguments(file, args, options);
validateTimeout(options.timeout, 'options.timeout');
validateTimeout(options.timeout);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. should we alternatively modify the validator function to use the name?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@Trott
Copy link
Member Author

Trott commented Mar 28, 2021

Landed in db7df59

@Trott Trott merged commit db7df59 into nodejs:master Mar 28, 2021
@Trott Trott deleted the superfluous branch March 28, 2021 14:56
ruyadorno pushed a commit that referenced this pull request Mar 29, 2021
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]>
ruyadorno pushed a commit that referenced this pull request Mar 30, 2021
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]>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants