★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21792Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

named anonymous functions in readline & zlib.js #21792

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@apapirovski apapirovski

@jasnell jasnell

@cjihrig cjihrig

@maclover7 maclover7

@hiroppy hiroppy

@trivikr trivikr

Assignees
No one assigned
Labels
author ready readline zlib
Projects
None yet
Milestone
No milestone
8 participants
@antsmartian @trivikr @apapirovski @maclover7 @jasnell @cjihrig @hiroppy @nodejs-github-bot
@antsmartian
Copy link
Contributor

antsmartian commented Jul 13, 2018 •
edited

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

@nodejs-github-bot nodejs-github-bot added readline zlib labels Jul 13, 2018

@apapirovski

apapirovski reviewed Jul 13, 2018
View changes

Copy link
Member

apapirovski left a 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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

antsmartian commented Jul 13, 2018

Yeah sure. Will do the same and update.
@antsmartian

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

antsmartian commented Jul 13, 2018

@apapirovski Ok, I did skimmed the source code of zlip.js, looks like we can use syncBufferWrapper, as this function just returns the zlibBufferSync and used for different compression formats like gzip, gunzip etc in sync mode. Similary, for zlibBuffer, we can name it as asyncBufferWrapper as the implementation of zlibBuffer is based on event emitters (async). Let me know if I'm wrong otherwise here.

For SIGCONT, I better rename the function to be continueProcess, as I guess that suits more natural. Thanks.

@antsmartian antsmartian force-pushed the antsmartian:anonymous_functions branch from 31d0635 to 6e0e792 Jul 14, 2018

@apapirovski

apapirovski approved these changes Jul 15, 2018
View changes

lib/readline.js
@@ -843,7 +843,7 @@ Interface.prototype._ttyWrite = function(s, key) {
if (this.listenerCount('SIGTSTP') > 0) {
this.emit('SIGTSTP');
} else {
process.once('SIGCONT', (function(self) {
process.once('SIGCONT', (function continueProcess(self) {

This comment has been minimized.

Sign in to view
Copy link
@apapirovski

apapirovski Jul 15, 2018

Member

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.

This comment has been minimized.

Sign in to view
Copy link
@antsmartian

antsmartian Jul 16, 2018

Author Contributor

Yes that make sense, may be will update the same in another PR. Thanks for your time on this.
@trivikr

This comment has been minimized.

Sign in to view
Copy link
Contributor

trivikr commented Jul 16, 2018

Thank you @antsmartian for your first PR in Node.js core!

The user.email needs to be updated in the commit as specified in this step
The current user.email is not registered with Github

@antsmartian antsmartian force-pushed the antsmartian:anonymous_functions branch from 6e0e792 to d8ec3d9 Jul 16, 2018

@antsmartian

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

antsmartian commented Jul 16, 2018

@trivikr Thanks, for some reason, my git config wasn't correct. Now fixed.
@trivikr

trivikr approved these changes Jul 17, 2018
View changes

@cjihrig

cjihrig approved these changes Jul 17, 2018
View changes

@hiroppy

hiroppy approved these changes Jul 17, 2018
View changes

@jasnell

jasnell approved these changes Jul 17, 2018
View changes

@trivikr

This comment has been minimized.

Sign in to view
Copy link
Contributor

trivikr commented Jul 18, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15928/
@apapirovski

This comment has been minimized.

Sign in to view
Copy link
Member

apapirovski commented Jul 29, 2018

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

@apapirovski apapirovski added the author ready label Jul 29, 2018

@maclover7

maclover7 approved these changes Jul 30, 2018
View changes

@maclover7

This comment has been minimized.

Sign in to view
Copy link
Member

maclover7 commented Jul 30, 2018

Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/16082/
@maclover7

This comment has been minimized.

Sign in to view
Copy link
Member

maclover7 commented Aug 2, 2018

Resumed once again, infrastructure issue on OSX: https://ci.nodejs.org/job/node-test-pull-request/16141/
@trivikr

This comment has been minimized.

Sign in to view
Copy link
Contributor

trivikr commented Aug 2, 2018

Landed in fc6f49a

@trivikr trivikr closed this Aug 2, 2018

trivikr added a commit that referenced this pull request Aug 2, 2018

@antsmartian @trivikr
readline,zlib: named anonymous functions 
PR-URL: #21792
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Loading status checks…
fc6f49a

targos added a commit that referenced this pull request Aug 2, 2018

@antsmartian @targos
readline,zlib: named anonymous functions 
PR-URL: #21792
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
76a6592

@antsmartian antsmartian deleted the antsmartian:anonymous_functions branch Aug 4, 2018

@rvagg rvagg referenced this pull request Aug 13, 2018

Merged

Release proposal: v10.9.0 #22295

This was referenced Aug 16, 2018

Closed
Closed
Closed

This was referenced Aug 16, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.