-
Notifications
You must be signed in to change notification settings - Fork 30k
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: edit for concision #17891
doc: edit for concision #17891
Conversation
This removes some wordy phrases (notably "it is important to note that") and a few instances of typographical excess (a bulleted list where separate paragraphs for each bullet item suffice, unneeded italics, etc.).
doc/api/async_hooks.md
Outdated
*Note*: It is important that `before`/`after` calls are unwound | ||
in the same order they are called. Otherwise an unrecoverable exception | ||
will occur and the process will abort. | ||
`before` and `after` calls are unwound in the same order they are 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.
I think this should be ..calls must be unwound...
since it's a note for the implementers.
doc/api/process.md
Outdated
An easy way to send the `SIGINT` signal is with `<Ctrl>-C` in most terminal | ||
programs. | ||
|
||
`SIGUSR1` is reserved by Node.js to start the [debugger][]. It's possible to |
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 actually prefer the list, it would also be easier to read if these are listed in a table.
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.
Restored the bulleted list. A table is a great idea if we can do it in a way that the information is still readable in raw markdown. If someone reading this wants to pick that piece up and make a separate PR, go for it. :-D
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.
LGTM modulo @joyeecheung 's suggestions.
@Trott i know you dislike the "note" parts a lot but I wonder if we might just discuss this again in general: If I read docs I want to know immediately what is important. Having a highlight per individual part makes therefore sense to me because I know what to expect, even when just glancing over the documentation. |
doc/api/process.md
Outdated
signal `0` can be used to test for the existence of a process. Sending `SIGINT`, | ||
`SIGTERM`, and `SIGKILL` cause the unconditional termination of the target | ||
process. | ||
An easy way to send the `SIGINT` signal is with `<Ctrl>-C` in most terminal |
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.
In this fragment we have:
<Ctrl>-C
CTRL+C
<Ctrl>+<Break>
Do we need to unify 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.
The two mentions of Ctrl-C are redundant, so I'd be inclined to remove this one. Which I'll do.
I made the other two instances consistent. Probably worth figuring out what (if any) universal standard there is for such things (like maybe see what RFCs do), document it in the style guide, and apply it across all docs. But not in this PR. :-D
doc/api/process.md
Outdated
install a listener but doing so will _not_ stop the debugger from starting. | ||
|
||
`SIGTERM` and `SIGINT` have default handlers on non-Windows platforms that | ||
resets the terminal mode before exiting with code `128 + signal number`. If |
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.
resets -> reset ?
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.
Yes, changed, thanks!
@BridgeAR You are right to caution against throwing out the proverbial baby with the bath water. I think we have four inter-related problems with our Note stuff.
|
@BridgeAR By the way, if you had your druthers, which of the four instances of Note this change set would be restored? I'm thinking the |
I agree that not all entries here are necessary but I do not see a negative impact of those "unnecessary" ones either. And I agree that reverting at least the |
I agree with @Trott that
This is exactly what I'd expect For example:
This whole API is mostly non-applicable to windows, seems like something you'd want to push in the user's face.
Seems like a useful hint, but not something that needs to be specially called out here.
If we reduce the Note: seems to stand out a bit better. |
Btw... the entire point of moving the |
@BridgeAR I've restored three of the four Note annotations that were originally removed. I removed the |
This removes some wordy phrases (notably "it is important to note that"). PR-URL: nodejs#17891 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Landed in 26607b8 |
This removes some wordy phrases (notably "it is important to note that"). PR-URL: #17891 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This removes some wordy phrases (notably "it is important to note that"). PR-URL: #17891 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This removes some wordy phrases (notably "it is important to note that"). PR-URL: #17891 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This removes some wordy phrases (notably "it is important to note that"). PR-URL: #17891 Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This removes some wordy phrases (notably "it is important to note that")
and a few instances of typographical excess (a bulleted list where
separate paragraphs for each bullet item suffice, unneeded italics,
etc.).
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
doc