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

lib: name anonymous callbacks #21412

Closed
wants to merge 4 commits into from
Closed

Conversation

mlrv
Copy link
Contributor

@mlrv mlrv commented Jun 19, 2018

First contribution here!

This commit is to help in the effort to name all anonymous
functions to help when heap debugging. Specifically, this commit
fixes some anonymous functions used as listeners in the lib/ folder.

Refs: #8913

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem. labels Jun 19, 2018
@@ -240,7 +240,7 @@ exports.translatePeerCertificate = function translatePeerCertificate(c) {
c.infoAccess = Object.create(null);

// XXX: More key validation?
info.replace(/([^\n:]*):([^\n]*)(?:\n|$)/g, function(all, key, val) {
info.replace(/([^\n:]*):([^\n]*)(?:\n|$)/g, function replacerCallback(all, key, val) {
Copy link
Member

Choose a reason for hiding this comment

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

Is that supposed to be replaceCallback instead of replacerCallback?

Copy link
Contributor Author

@mlrv mlrv Jun 20, 2018

Choose a reason for hiding this comment

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

It wasn't a typo in my head. I suppose that it makes sense in both versions:

  • replacerCallback -> the replacer function (inside replace)
  • replaceCallback -> the callback of the replace function

Happy to change it of course

Copy link
Member

Choose a reason for hiding this comment

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

Nah, if it wasn't a typo, it's fine with me. Just wanted to give you the opportunity to fix it up if it was a mistake.

Trott
Trott previously approved these changes Jun 19, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@trivikr
Copy link
Member

trivikr commented Jun 20, 2018

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 20, 2018
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Thanks for doing this and following up! 🎉

@mlrv
Copy link
Contributor Author

mlrv commented Jun 20, 2018

Glad to see it all seems to go well :)

If I understood correctly, the standard process is to wait at least 48 hours before merging, am I right?

Is there anything else that needs to be done?

@benjamingr
Copy link
Member

@mlrv that is correct, we can fast track this one though since it's naming rather than functionality. This is all explained here: https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#waiting-for-approvals . It also explicitly states that since this is a good-first-issue then fast-tracking might be appropriate.

Collaborators 👍 this comment to support fast-tracking this change.

@BridgeAR BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 20, 2018
@Trott
Copy link
Member

Trott commented Jun 20, 2018

CI again to try to confirm Windows failure is unlikely to be related: https://ci.nodejs.org/job/node-test-pull-request/15538/

@Trott
Copy link
Member

Trott commented Jun 21, 2018

Hmmm... let's try a total re-run:

CI: https://ci.nodejs.org/job/node-test-pull-request/15555/

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Hi @mlrv — thank you for working on this. In my opinion naming should provide some value, rather than just being decorative. Names such as socketOnceCallback do not help anyone and do not make the stack trace any more descriptive than an anonymous function would. I've left some comments regarding some potentially more useful names.

@@ -721,7 +721,7 @@ Readable.prototype.pipe = function(dest, pipeOpts) {
};

function pipeOnDrain(src) {
return function() {
return function pipeOnDrainInner() {
Copy link
Member

Choose a reason for hiding this comment

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

pipeOnDrainCallback? It won't be "inner" by the time it's in a stack trace.

@@ -185,7 +185,7 @@ OutgoingMessage.prototype.setTimeout = function setTimeout(msecs, callback) {
}

if (!this.socket) {
this.once('socket', function(socket) {
this.once('socket', function socketOnceCallback(socket) {
Copy link
Member

Choose a reason for hiding this comment

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

socketSetTimeoutOnConnect?

@@ -202,7 +202,7 @@ OutgoingMessage.prototype.destroy = function destroy(error) {
if (this.socket) {
this.socket.destroy(error);
} else {
this.once('socket', function(socket) {
this.once('socket', function socketOnceCallback(socket) {
Copy link
Member

Choose a reason for hiding this comment

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

socketDestroyOnConnect?

@@ -240,7 +240,7 @@ exports.translatePeerCertificate = function translatePeerCertificate(c) {
c.infoAccess = Object.create(null);

// XXX: More key validation?
info.replace(/([^\n:]*):([^\n]*)(?:\n|$)/g, function(all, key, val) {
info.replace(/([^\n:]*):([^\n]*)(?:\n|$)/g, function replacerCallback(all, key, val) {
Copy link
Member

Choose a reason for hiding this comment

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

Switch to an anonymous function instead of naming it? The function name is not particularly useful.

Copy link
Member

Choose a reason for hiding this comment

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

I thought there was consensus that naming anonymous callback functions was helpful. If that's not the case, I should update #8913 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apapirovski any update on this?

Copy link
Member

Choose a reason for hiding this comment

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

My bad here as far as dropping the ball and misphrasing. What I meant to say is: I think this should just be an arrow function. I don't think we gain much by naming callbacks for things like String.prototype.replace.

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 implemented the suggestions and fixed the conflicts. Happy to go ahead if all the checks are green?

@mlrv
Copy link
Contributor Author

mlrv commented Jun 26, 2018

Hi @apapirovski, thanks for the feedback, I totally see your point. Happy to change the names if you think those could be more helpful. On the last comment, not sure I agree with you, what do other people think?

@addaleax addaleax removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. labels Jul 13, 2018
@maclover7 maclover7 added the wip Issues and PRs that are still a work in progress. label Jul 28, 2018
@@ -240,7 +240,7 @@ exports.translatePeerCertificate = function translatePeerCertificate(c) {
c.infoAccess = Object.create(null);

// XXX: More key validation?
info.replace(/([^\n:]*):([^\n]*)(?:\n|$)/g, function (all, key, val) {
info.replace(/([^\n:]*):([^\n]*)(?:\n|$)/g, function(all, key, val) {
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest just using (all, key, val) => { here.

@mlrv
Copy link
Contributor Author

mlrv commented Jul 30, 2018

@benjamingr all green, good to go? :)

@benjamingr benjamingr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 30, 2018
@Trott
Copy link
Member

Trott commented Aug 1, 2018

@Trott Trott dismissed their stale review August 1, 2018 01:25

not sure about the arrow function fitting in with the rest but won't block

@Trott
Copy link
Member

Trott commented Aug 1, 2018

Weird. CI failed on git due to a conflict. Let's try again...

CI: https://ci.nodejs.org/job/node-test-pull-request/16105/

@Trott
Copy link
Member

Trott commented Aug 1, 2018

GitHub says this has no conflicts with the base branch, but manually trying to apply the changes shows conflicts. I'll resolve them, push back up, and restart CI.

mlrv added 4 commits July 31, 2018 18:37
This commit is to help in the effort to name all anonymous
functions to help when heap debugging. Specifically, this commit
fixes some anonymous functions used as listeners in the lib/ folder.

Refs: nodejs#8913
@Trott
Copy link
Member

Trott commented Aug 1, 2018

@Trott
Copy link
Member

Trott commented Aug 1, 2018

Might be good for people to double-check the changes now that I've done a rebase and resolved conflicts.

@maclover7 maclover7 removed the wip Issues and PRs that are still a work in progress. label Aug 3, 2018
@Trott
Copy link
Member

Trott commented Aug 3, 2018

maclover7 pushed a commit that referenced this pull request Aug 4, 2018
This commit is to help in the effort to name all anonymous
functions to help when heap debugging. Specifically, this commit
fixes some anonymous functions used as listeners in the lib/ folder.

PR-URL: #21412
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Refs: #8913
@maclover7
Copy link
Contributor

Landed in d7496bf, congrats on your first PR to Node.js!
❤️ 💚 💙 💛 💜

@maclover7 maclover7 closed this Aug 4, 2018
targos pushed a commit that referenced this pull request Aug 6, 2018
This commit is to help in the effort to name all anonymous
functions to help when heap debugging. Specifically, this commit
fixes some anonymous functions used as listeners in the lib/ folder.

PR-URL: #21412
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Refs: #8913
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. http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.