-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Conversation
@@ -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*/ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
LGTM |
@@ -51,7 +51,8 @@ var server = net.createServer((socket) => { | |||
}); | |||
|
|||
// grab a random port. | |||
server.listen(() => { | |||
server.listen((err) => { | |||
if(err) throw err; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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>
Fixed squashed and update commit message. Thanks for the feedback. |
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: #4834 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Roman Klauke <romaaan.git@gmail.com>>
Thanks! Landed in 83e43fb. There were a few trailing whitespace errors which I fixed. |
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>>
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>>
This pr is blocked on being backported until other doc changes make it to LTS |
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>>
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>>
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>>
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>>
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.