-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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: fix typos #8370
doc: fix typos #8370
Conversation
Looks good to me! |
LGTM |
2 similar comments
LGTM |
LGTM |
@@ -69,7 +69,7 @@ actually uses - are those above._ | |||
|
|||
* **timers**: this phase executes callbacks scheduled by `setTimeout()` | |||
and `setInterval()`. | |||
* **I/O callbacks**: most types of callback except timers, `setImmedate()`, close | |||
* **I/O callbacks**: most types of callback except timers, `setImmediate()`, 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.
Does this sentence look complete? I am not able to understand this.
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 didn't read the context but as is I also can't understand it.
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 think this is supposed to read as: all callbacks, but not the ones listed above under timers, the setImmediate()
ones, and those listed under close callbacks which are run later…? But ack, it isn’t very clear…
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 agree, reading the full list one can infer something like this:
"executes almost all callbacks with the exception of close callbacks, the ones scheduled by timers and setImmediate()
"
It is a long sentence but I think it's better to be explicit.
Changes look fine, LGTM. |
LGTM |
PR-URL: #8370 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@addaleax yes, will do. |
PR-URL: nodejs#8370 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #8370 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: #8370 (diff) PR-URL: #8400 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Refs: #8370 (diff) PR-URL: #8400 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Checklist
Affected core subsystem(s)
doc
Description of change
Fix typos / spelling errors in doc/api and doc/topics