named anonymous functions in readline & zlib.js#21792
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added readline 
apapirovski left a comment
apapirovski left a comment There was a problem hiding this comment.
Hi @antsmartian, the goal of naming these functions should be to make a more useful name than the anonymous one. Please try to consider the use cases for these functions and name based on that. In particular this applies to fnWrap.
antsmartian commented Jul 13, 2018
Yeah sure. Will do the same and update. |
antsmartian commented Jul 13, 2018
@apapirovski Ok, I did skimmed the source code of For |
antsmartianforce-pushed the anonymous_functions branch from 31d0635 to 6e0e792Compare
lib/readline.js Outdated
There was a problem hiding this comment.
So we could actually do a slightly better thing here. Instead of the IIFE, we could use an arrow function and replace self with this — unless I'm missing something. Then we will just have a normal event callback instead of this weird thing it is right now. Feel free to either do that change here or in a separate PR.
There was a problem hiding this comment.
Yes that make sense, may be will update the same in another PR. Thanks for your time on this.
trivikr commented Jul 16, 2018
Thank you @antsmartian for your first PR in Node.js core! The |
antsmartianforce-pushed the anonymous_functions branch from 6e0e792 to d8ec3d9Compareantsmartian commented Jul 16, 2018
@trivikr Thanks, for some reason, my git config wasn't correct. Now fixed. |
trivikr commented Jul 18, 2018
apapirovski commented Jul 29, 2018
apapirovski added the author ready 
maclover7 commented Jul 30, 2018
maclover7 commented Aug 2, 2018
Resumed once again, infrastructure issue on OSX: https://ci.nodejs.org/job/node-test-pull-request/16141/ |
trivikr commented Aug 2, 2018
Landed in fc6f49a |
PR-URL: #21792 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]>
PR-URL: #21792 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]>
8 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes