-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: add comment for net.Server's error event #11136
Conversation
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.
Looks good, with a small nit.
Two things regarding the commit message though:
-
Your author name in this commit is given as “q”. Is that intended or do you prefer to be listed (changelog, git log, AUTHORS file) with some other name? People typically prefer their full name, but ultimately it’s up to you.
-
Could you use
Fixes:
instead ofFix:
? 😄 It’s not important, but the former is more commonly used around here.
doc/api/net.md
Outdated
@@ -40,8 +40,9 @@ added: v0.1.90 | |||
|
|||
* {Error} | |||
|
|||
Emitted when an error occurs. The [`'close'`][] event will be called directly | |||
following this event. See example in discussion of `server.listen`. | |||
Emitted when an error occurs. Unlike the [`net.Socket`][], the [`'close'`][] |
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 drop the
, or replace it with with
? (not a native speaker myself but the the
sounds odd here)
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.
Of course, thank you!
ef32fb1
to
5cbd17a
Compare
doc/api/net.md
Outdated
following this event. See example in discussion of `server.listen`. | ||
Emitted when an error occurs. Unlike [`net.Socket`][], the [`'close'`][] | ||
event will NOT be called directly following this event, unless you call | ||
`server.close()` manually. See example in discussion of `server.listen`. |
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.
Please avoid the use of pronouns like you
in the documentation. Also, please use either italics or bold for emphasis as opposed to upper case (e.g. NOT
) This can be reworded as:
Emitted when an error occurs., Unlike [`net.Socket'`[], the [`'close'`][]
event will **not** be called directly following this event unless
`server.close()` is manually called. See the example in the discussion
of `server.listen()`.
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.
Thanks.
The net.Socket is not an event, so can not be displayed as a link by ['net.Socket'
][], this is why i just use [net.Socket
][].
Besides, "See the example in the discussion", two "the" seems odd? So i stripped second "the".
See my second commit's preview: https://github.com/sjitech/node/blob/3b440d3db2a2dfa9833eb06ede6f1adb909c1644/doc/api/net.md#event-error
The |
doc/api/net.md
Outdated
Emitted when an error occurs. The [`'close'`][] event will be called directly | ||
following this event. See example in discussion of `server.listen`. | ||
Emitted when an error occurs. Unlike [`net.Socket`][], the [`'close'`][] | ||
event will **not** be called directly following this event unless |
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.
events are "emitted", not "called"
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.
ok, i will correct it and the original description.
Yes. |
sorry, i found the
So i think no need to add description of |
doc/api/net.md
Outdated
Emitted when an error occurs. Unlike [`net.Socket`][], the [`'close'`][] | ||
event will **not** be emitted directly following this event unless | ||
`server.close()` is manually called. See the example in discussion of | ||
`server.listen()`. |
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.
would be good to link server.close() and server.listen()
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.
@QianJin2013 ... please see @sam-github's comment above :-)
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.
@sam-github I'd tried to link server.close() and server.listen() but failed. Now i know the reason, i have modified doc again.
you'll need to squash and rebase |
@sam-github yes, i will squash and rebase it. |
6eb63c9
to
72b4bd3
Compare
72b4bd3
to
3dc2a1d
Compare
Landed in 52ddb41. thanks. |
Fix: #9710
Checklist
Affected core subsystem(s)
doc