-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: make the style of notes consistent #13133
Conversation
@@ -115,7 +115,7 @@ if (cluster.isMaster) { | |||
d.on('error', (er) => { | |||
console.error(`error ${er.stack}`); | |||
|
|||
// Note: we're in dangerous territory! | |||
// Note: We're in dangerous territory! |
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.
Note: that falls outside the scope of this PR, but "we" shouldn't have been used here at all. I'll reword all of the comments in this example in another PR.
doc/api/events.md
Outdated
@@ -142,7 +142,7 @@ myEmitter.emit('error', new Error('whoops!')); | |||
|
|||
To guard against crashing the Node.js process, a listener can be registered | |||
on the [`process` object's `uncaughtException` event][] or the [`domain`][] module | |||
can be used. (_Note, however, that the `domain` module has been deprecated_) | |||
can be used. _(Note, however, that the `domain` module has been deprecated.)_ |
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.
And what about this case, to my perception, this is not a note as in "Note:", but rather just a regular sentence. I fixed the stylistic mistakes while I was here, though.
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.
Agreed. AFAICT this is an editorial comment that happens to start with the word "note".
But it should be "Note however," no first comma.
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 am not a native speaker, but as for me, the punctuation around "however" is pretty valid here as is.
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 that Note, however, that...
is perfectly fine (and to my sense, preferable). I do think that note should probably _not* be italicized, though.
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.
Ack.
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.
Rubber stamp completeness...
doc/STYLE_GUIDE.md
Outdated
@@ -58,6 +58,11 @@ | |||
* References to constructor functions should use PascalCase | |||
* References to constructor instances should be camelCased | |||
* References to methods should be used with parentheses: `socket.end()` instead of `socket.end` | |||
* In order to make a note that a reader should draw special attention 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 think this would be somewhat better as To draw special attention to a note,
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.
Done, thanks.
doc/api/net.md
Outdated
|
||
* All [`net.Socket`][] are set to `SO_REUSEADDR` (See [socket(7)][] for |
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.
Nit: I understand this goes against the style defined above but isn't it better to keep the bulleted list here instead of repeating Note: in two consecutive lines?
doc/api/tls.md
Outdated
generated from `process.argv`, other APIs that create secure contexts | ||
have no default value. | ||
|
||
**Note:** [`tls.createServer()`][] sets the default value of the |
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.
Nit: it might makes sense to also use a bulleted list here.
ping @nodejs/collaborators Before we really start enforcing it, I think we need to make sure that others are acknowledged about this and I'd like to hear more opinions about the preferred style, specifically re: questions like bold vs italic, should we group consequent notes into unordered lists, etc. |
+1 on this, and it would be nicer if we could have this addition as one of the actual link rules. |
doc/STYLE_GUIDE.md
Outdated
@@ -58,6 +58,10 @@ | |||
* References to constructor functions should use PascalCase | |||
* References to constructor instances should be camelCased | |||
* References to methods should be used with parentheses: `socket.end()` instead of `socket.end` | |||
* To draw special attention to a note, adhere to the following guidelines: |
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.
To be quite honest, I generally prefer the *Note*:
style and have been incrementally working towards that for quite some time. I guess this is fine tho. Mildly -0
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.
As I said,
I don't have any strong opinions about the notes style except that it should be only one, so the formatting may be discussed in this PR.
I am completely fine with changing the style to what you prefer, a Vim macro will chew it easily 😄
@watilde yeah, I am really looking forward to see the markdown linter landed. Sure, we can make a rule for that. Aside from this PR, ensuring that we use 80-character wrapping (except changelogs) and em-dashes are on my to-do list. |
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
Make the style of "Note:" paragraphs consistent and document the guidelines in `doc/STYLE_GUIDE.md`. Fixes: nodejs#13131
16b3231
to
f7db525
Compare
I've updated the PR. The first commit is the original commit rebased onto master with conflicts resolved and with a couple of notes that I missed before (or that weren't present before the rebase, idk) updated to conform to the general style. The second commit changes the style to what @jasnell prefers and the third commit follows the suggestion by @lpinca to use bulleted lists for consecutive notes. @refack @lpinca @watilde @mhdawson can you please take another look and say whether this still LGTY? Thanks! |
-0.5 on Waiting for consensus on bullet lists for https://github.com/nodejs/node/pull/13173/files#diff-acabf706a8aa070a8796e3573f7e4678 |
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.
Nice, thanks.
Still LGTM. |
|
Reopened, seems like I was too fast here. |
@jasnell how strongly do you feel on the |
I’m +1 to |
I yield. I see two explicit +1 for |
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.
👍 to *Note*:
style.
|
Make the style of "Note:" paragraphs consistent and document the guidelines in `doc/STYLE_GUIDE.md`. PR-URL: nodejs#13133 Fixes: nodejs#13131 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Landed in 2af49b6 |
Make the style of "Note:" paragraphs consistent and document the guidelines in `doc/STYLE_GUIDE.md`. PR-URL: #13133 Fixes: #13131 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported |
Make the style of "Note:" paragraphs consistent and document the guidelines in
doc/STYLE_GUIDE.md
.Fixes: #13131
I picked the style that I find reasonable and which I had used once adding a note myself, but I don't have any strong opinions about the notes style except that it should be only one, so the formatting may be discussed in this PR.
Checklist
Affected core subsystem(s)
doc