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.

Sign up for GitHub

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

Jump to bottom

name net anonymous functions #9357

Closed
wants to merge 2 commits into from
Closed

name net anonymous functions #9357

wants to merge 2 commits into from

Conversation

pvsousalima
Copy link
Contributor

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

net

Description of change

net: name anonymous functions in net module

name anonymous functions in net module
the changes are related to Ref: #8913
regarding the naming of just the inline anonymous
functions that are not assigned to a variable

@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Oct 29, 2016
@pvsousalima pvsousalima changed the title Name anonymous net name net anonymous functions Oct 29, 2016
@Trott
Copy link
Member

Trott commented Oct 29, 2016

LGTM if CI is ✅

jasnell
jasnell approved these changes Oct 30, 2016
View reviewed changes
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

imyller
imyller approved these changes Oct 31, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

mcollina
mcollina approved these changes Oct 31, 2016
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@pvsousalima
Copy link
Contributor Author

Hopefully CI will go 👍

@mcollina
Copy link
Member

mcollina commented Nov 1, 2016

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

@pvsousalima
Copy link
Contributor Author

pvsousalima commented Nov 1, 2016 •
edited
Loading

Status aborted 😢 similar to build #4743 on CI: I guess it should work on top of 6ef636c however the problem doesn't look to be related to the changes

@pvsousalima
net: name anonymous functions in net module
2321c4c 
the changes are related to Ref: #8913
regarding the naming of just the inline anonymous
functions that are not assigned to a variable
lpinca
lpinca reviewed Nov 1, 2016
View reviewed changes
lib/net.js
@@ -1389,7 +1389,7 @@ Server.prototype.listen = function() {
};

function lookupAndListen(self, port, address, backlog, exclusive) {
require('dns').lookup(address, function(err, ip, addressType) {
require('dns').lookup(address, function emitDnsLookup(err, ip, addressType) {
Copy link
Member

@lpinca lpinca Nov 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Can I ask why you chose this name? I see no emit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was under dns context, however it should be a listen or doListening. I will modify on next commit :) @lpinca

Copy link
Contributor Author

@pvsousalima pvsousalima Nov 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I put it on 6ce4862 probably now LGTY (looks good to you) 😄

@mcollina
Copy link
Member

mcollina commented Nov 1, 2016

The build is fine: https://ci.nodejs.org/job/node-test-commit/5887/.
freebsd failure :(.

@pvsousalima
Copy link
Contributor Author

pvsousalima commented Nov 1, 2016 •
edited
Loading

6ef636c wasn't supposed to fix it? I did the rebase as well @mcollina

@mcollina
Copy link
Member

mcollina commented Nov 1, 2016

@pvsousalima the failure was unrelated, I prefer not to waste precious resources on the testing env. A full CI run takes forever.

@pvsousalima
net: name anonymous functions in net module
6ce4862 
changing the name proposed to the anonymous function
as stated on #9357 (comment)
@silverwind
Copy link
Contributor

Thanks! Landed in d1785d9.

@silverwind silverwind closed this Nov 1, 2016
silverwind pushed a commit that referenced this pull request Nov 1, 2016
@pvsousalima @silverwind
net: name anonymous functions
d1785d9 
the changes are related #8913
regarding the naming of just the inline anonymous
functions that are not assigned to a variable

PR-URL: #9357
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@Trott
Copy link
Member

Trott commented Nov 1, 2016

6ef636c wasn't supposed to fix it? I did the rebase as well @mcollina

6ef636c fixed failing tests. What you saw here was a Jenkins connection failure. It never even got to the point of running tests because the host and Jenkins stopped communicating during the build. If anyone knows how to fix that, I'm sure the Build WG (#node-build on Freenode IRC, https://github.com/nodejs/build on GitHub) would love to hear it.

@pvsousalima
Copy link
Contributor Author

pvsousalima commented Nov 1, 2016 •
edited
Loading

Is there a way to debug the communication process @Trott ? I want to start at that path 😄

@Trott
Copy link
Member

Trott commented Nov 2, 2016

Is there a way to debug the communication process @Trott ?

I don't know. If you know Jenkins administration well and want to discuss it, create an issue at https://github.com/nodejs/build/issues.

evanlucas pushed a commit that referenced this pull request Nov 3, 2016
@pvsousalima @evanlucas
net: name anonymous functions
d09eb9c 
the changes are related #8913
regarding the naming of just the inline anonymous
functions that are not assigned to a variable

PR-URL: #9357
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@evanlucas evanlucas mentioned this pull request Nov 3, 2016
Merged
@MylesBorins MylesBorins added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 8, 2017
@piascikj piascikj mentioned this pull request Oct 3, 2017
Open
@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
Reviewers

@lpinca lpinca lpinca left review comments

@targos targos targos approved these changes

@imyller imyller imyller approved these changes

@jasnell jasnell jasnell approved these changes

@silverwind silverwind silverwind approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@pvsousalima @Trott @mcollina @silverwind @jasnell @lpinca @imyller @targos @MylesBorins @nodejs-github-bot