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

net: move debug statement #12616

Closed
wants to merge 1 commit into from
Closed

net: move debug statement #12616

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Apr 24, 2017 •
edited
Loading

This will allow localAddress to be properly set before writing debug output.

/cc @bnoordhuis

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

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

@mscdex
net: move debug statement
212f721 
This will allow `localAddress` to be properly set before writing
debug output.
@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Apr 24, 2017
bnoordhuis
bnoordhuis reviewed Apr 24, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

You can move it a few lines down to line 875 to avoid the duplication and, optionally, include err in the message.

@mscdex
Copy link
Contributor Author

mscdex commented Apr 24, 2017

I figured I'd keep the original message intact since it says 'binding' instead of 'bound'. *shrug* Unless we want to have two separate messages?

@bnoordhuis
Copy link
Member

I figured I'd keep the original message intact since it says 'binding' instead of 'bound'.

Feel free to change it. There is no backwards compatibility concern here.

cjihrig
cjihrig approved these changes Apr 25, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, although I slightly prefer @bnoordhuis suggestion.

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Aug 26, 2017
@mscdex @BridgeAR
net: move debug statement
2154a3c 
This will allow `localAddress` to be properly set before writing
debug output.

PR-URL: nodejs#12616
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

Landed in 2154a3c. I went ahead to move the statement down as @bnoordhuis suggested. I hope that is ok.

@BridgeAR BridgeAR closed this Aug 26, 2017
@mscdex
Copy link
Contributor Author

mscdex commented Aug 26, 2017

@BridgeAR If the statement was going to be moved at the very least the wording should have been changed to reflect what actually happened...

@BridgeAR
Copy link
Member

@mscdex I am not sure I can follow. As far as I see it the wording still fits.

@mscdex
Copy link
Contributor Author

mscdex commented Aug 26, 2017 •
edited
Loading

@BridgeAR Since the message is now printed after bind() is called, it's now already bound. If there was an error, you wouldn't see that message. binding seems more appropriate for before the bind() is called ("binding" as in "I am about to bind").

@BridgeAR
Copy link
Member

Oh, of course. Sorry, I should probably not have changed that especially not without changing the wording. I guess it is to late for a force push but I can open a fix for it later.

@BridgeAR
Copy link
Member

If you think it is not yet to late for a force push, maybe you could just do that right quick? I will be afk for a while.

@mscdex mscdex deleted the net-fix-debug-output branch August 26, 2017 12:59
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
@mscdex
net: move debug statement
269fe15 
This will allow `localAddress` to be properly set before writing
debug output.

PR-URL: nodejs/node#12616
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
@mscdex
net: move debug statement
d79582b 
This will allow `localAddress` to be properly set before writing
debug output.

PR-URL: nodejs/node#12616
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@mscdex @MylesBorins
net: move debug statement
4dbf6ac 
This will allow `localAddress` to be properly set before writing
debug output.

PR-URL: #12616
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@mscdex @MylesBorins
net: move debug statement
b4e8850 
This will allow `localAddress` to be properly set before writing
debug output.

PR-URL: #12616
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell 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.
8 participants
@mscdex @bnoordhuis @BridgeAR @MylesBorins @jasnell @lpinca @cjihrig @nodejs-github-bot