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

doc: check for errors in listen event #4834

Closed
wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

In the docs we typically check for errors and surface them. This is IMO a good
idea and good practice. This PR adds a check for errors in three places in
the net docs where it was missing.

@r-52 r-52 added doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem. labels Jan 24, 2016
@@ -672,7 +674,10 @@ To listen on the socket `/tmp/echo.sock` the third line from the last would
just be changed to

```js
server.listen('/tmp/echo.sock', () => { /* 'listening' listener*/ })
server.listen('/tmp/echo.sock', (err) => {
/* 'listening' listener*/
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here, could you fix both comments here and above to be single line comments on its own line?

// 'listening' listener

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I fixed a couple other inline comments to be on following lines as well.

@silverwind
Copy link
Contributor

LGTM

@@ -51,7 +51,8 @@ var server = net.createServer((socket) => {
});

// grab a random port.
server.listen(() => {
server.listen((err) => {
if(err) throw err;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a space between if and (. And preferably put throw err; on a separate line. Same comment on the other changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm also +1 on putting it on the separate line. I can go ahead and do that but it means inconsistencies with other docs. A lot of other docs use if (err) throw err; on a single line.

Do we want if (err) checks to not be single line in the docs?

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal preference is for a separate line, but do whatever is currently more consistent. I think the docs working group is planning to add linting for this kind of thing in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do whatever the linter is fine with and what's most consistent in that file. We have a rule in place for the space after if, but do not require things like the curly rule yet.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 24, 2016

LGTM

In the docs we typically check for errors and surface them. This
is IMO a good idea and good practice. This PR adds a check for
errors in three places in the `net` docs where it was missing.

PR-URL: nodejs#4834
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewd-By: Colin Ihrig <cjihrig@gmail.com>
@benjamingr
Copy link
Member Author

Fixed squashed and update commit message. Thanks for the feedback.

@r-52
Copy link
Contributor

r-52 commented Jan 24, 2016

LGTM

silverwind pushed a commit that referenced this pull request Jan 24, 2016
In the docs we typically check for errors and surface them. This
is IMO a good idea and good practice. This PR adds a check for
errors in three places in the `net` docs where it was missing.

PR-URL: #4834
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>>
@silverwind
Copy link
Contributor

Thanks! Landed in 83e43fb. There were a few trailing whitespace errors which I fixed.

@silverwind silverwind closed this Jan 24, 2016
rvagg pushed a commit that referenced this pull request Jan 25, 2016
In the docs we typically check for errors and surface them. This
is IMO a good idea and good practice. This PR adds a check for
errors in three places in the `net` docs where it was missing.

PR-URL: #4834
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>>
benjamingr added a commit to benjamingr/io.js that referenced this pull request Jan 27, 2016
In the docs we typically check for errors and surface them. This
is IMO a good idea and good practice. This PR adds a check for
errors in three places in the `net` docs where it was missing.

PR-URL: nodejs#4834
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>>
@MylesBorins
Copy link
Contributor

This pr is blocked on being backported until other doc changes make it to LTS

MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
In the docs we typically check for errors and surface them. This
is IMO a good idea and good practice. This PR adds a check for
errors in three places in the `net` docs where it was missing.

PR-URL: #4834
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
In the docs we typically check for errors and surface them. This
is IMO a good idea and good practice. This PR adds a check for
errors in three places in the `net` docs where it was missing.

PR-URL: #4834
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
In the docs we typically check for errors and surface them. This
is IMO a good idea and good practice. This PR adds a check for
errors in three places in the `net` docs where it was missing.

PR-URL: #4834
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
In the docs we typically check for errors and surface them. This
is IMO a good idea and good practice. This PR adds a check for
errors in three places in the `net` docs where it was missing.

PR-URL: nodejs#4834
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Klauke <romaaan.git@gmail.com>>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants