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: name anonymous functions #9880

Closed

Conversation

brad-decker
Copy link
Contributor

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

child_process

Description of change

Per #8913 i've named anonymous callback handlers.

I have some mixed styling in here because i'm still going over the message log from #8913 and trying to determine best practices. Is the correct implementation of current anonymous functions just to apply a name to those functions without lifting them out of the function call and using arrow functions?

In particular this file has no arrow functions in it yet, so i would assume we'd want to stick to using:

setTimeout(function name() {}, options.timeout);

over

const name = () => {};
setTimeout(name, options.timeout);

if that's the case is there a preferred naming schema for things like setTimeout? The names on some of these seem to be moving towards the verbose, ex:

child.stderr.addListener('data', function stderrAddListenerCallback(){});

Definitely ready to make changes to this to make it compliant.

@nodejs-github-bot nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Dec 1, 2016
}

if (child.stdout) {
if (encoding)
child.stdout.setEncoding(encoding);

child.stdout.addListener('data', function(chunk) {
Copy link
Contributor

@mscdex mscdex Dec 1, 2016

Choose a reason for hiding this comment

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

Why not name the function inline here instead?:

child.stdout.addListener('data', function onChildStdout(chunk) {

or even shorter:

child.stdout.on('data', function onChildStdout(chunk) {

@@ -247,17 +247,18 @@ exports.execFile = function(file /*, args, options, callback*/) {
}

if (options.timeout > 0) {
timeoutId = setTimeout(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, this could be:

timeoutId = setTimeout(function delayedKill() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

}

if (child.stderr) {
if (encoding)
child.stderr.setEncoding(encoding);

child.stderr.addListener('data', function(chunk) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@mscdex
Copy link
Contributor

mscdex commented Dec 1, 2016

I think arrow functions are fine for the most part, but if they result in the addition of new lines of code, I don't think it's really worth it at that point. Since the purpose is to give better stack traces, just adding a name to the anonymous functions is easiest and requires less change.

@brad-decker
Copy link
Contributor Author

@mscdex Thanks for the feedback, working on making those changes now. One question,

var _deprecatedCustomFds = internalUtil.deprecate(function deprecateCustomFds(options) {
  options.stdio = options.customFds.map(function(fd) {
    return fd === -1 ? 'pipe' : fd;
  });
}, 'child_process: options.customFds option is deprecated. ' +
   'Use options.stdio instead.');

this first line is way too long. Do you happen to have a recommendation on function name for this particular callback? Or should I just break to a new line to follow linting and leave a verbose function name?

@mscdex
Copy link
Contributor

mscdex commented Dec 1, 2016

Without checking with the linter, I'd be fine with something like:

const _deprecatedCustomFds =
  internalUtil.deprecate(function deprecateCustomFds(options) {

or

const _deprecatedCustomFds = internalUtil.deprecate(
  function deprecateCustomFds(options) {

@brad-decker
Copy link
Contributor Author

@mscdex suggested changes implemented. Thanks for your feedback.

@mscdex
Copy link
Contributor

mscdex commented Dec 1, 2016

LGTM

@Trott
Copy link
Member

Trott commented Dec 1, 2016

@brad-decker
Copy link
Contributor Author

Looks like 3 checks failed. Having a little confusion with navigating the CI results to find the issues/logs to aid in correcting these failures. @mscdex @Trott

@Trott
Copy link
Member

Trott commented Dec 2, 2016

@brad-decker Looks like somebody thought there was something wrong with CI and terminated a whole bunch of jobs, including this one. (I had started a lot of them because: Code & Learn.) Bummer. Oh well. Here's another one: https://ci.nodejs.org/job/node-test-pull-request/5096/

'Use options.stdio instead.');
const _deprecatedCustomFds = internalUtil.deprecate(
function deprecateCustomFds(options) {
options.stdio = options.customFds.map(function(fd) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe use an arrow function for this? It should fit in a single line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lpinca The goal would be getting rid of the anonymous callback functions, so if we were to go the route of an arrow function it'd be declaring a new const above this declaration that defines the callback function, then passing that const to internalUtil.deprecate. I'm 👍 on doing that, but I want to make sure that that would fit the overall code style/design patterns we're wanting to achieve.

Copy link
Member

Choose a reason for hiding this comment

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

@brad-decker yup, the reason for my comment was that the function was still anonymous.
Maybe it's better to just give it a name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually thinking you were talking about changing

function deprecateCustomFds(options) {

To a const, so i misunderstood you. totally missed the call to map.

const mapCustomFds = (fd) => { return fd === -1 ? 'pipe' : fd; };
const _deprecatedCustomFds = internalUtil.deprecate(
  function deprecateCustomFds(options) {
    options.stdio = options.customFds.map(mapCustomFds);
  }, 'child_process: options.customFds option is deprecated. ' +
     'Use options.stdio instead.'
);

I wrapped the block for the arrow function in curlys to avoid lint error for no-confusing-arrow. Does this solution look okay to you?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I would only remove the return statement.

const mapCustomFds = (fd) => fd === -1 ? 'pipe' : fd;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when removing the return statement i get the no-confusing-arrow lint rule being thrown complaining about using arrow functions ambigiously with conditional expressions.

Copy link
Member

Choose a reason for hiding this comment

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

Oh ops, sorry so maybe just give the original function the mapCustomFds name without using the arrow function.

I think it's the cleanest thing to do.

Sorry again for the confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! Thanks for the feedback. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in new commit and rebased/squashed

@@ -70,10 +70,10 @@ exports._forkChild = function(fd) {
p.open(fd);
p.unref();
const control = setupChannel(process, p);
process.on('newListener', function(name) {
process.on('newListener', function newListenerCallback(name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to ignore, but perhaps name this onNewListener. Same with onRemoveListener.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i think onNewListener is better naming convention. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in new commit and rebased.

Per nodejs#8913 i've named anonymous callback handlers,
This commit is just a first stab at this looking
for community feedback on style/naming conventions
@Trott Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 5, 2016
@Trott
Copy link
Member

Trott commented Dec 5, 2016

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the PR and for participating in the code-and-learn!

@brad-decker
Copy link
Contributor Author

Thanks @jasnell

jasnell pushed a commit that referenced this pull request Dec 5, 2016
Refs: #8913
PR-URL: #9880
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
@jasnell
Copy link
Member

jasnell commented Dec 5, 2016

Landed in bbed075. Thank you!

@jasnell jasnell closed this Dec 5, 2016
Fishrock123 pushed a commit that referenced this pull request Dec 6, 2016
Refs: #8913
PR-URL: #9880
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
Refs: nodejs#8913
PR-URL: nodejs#9880
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 8, 2017
@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants