-
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: update tools/doc/README.md #20047
Conversation
Fixed my own typo. |
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 okay, but IMHO this file could really do with some work -- it's basically one example but without explanations of things like the YAML metadata (e.g., REPLACEME
). Maybe some sort of reconciliation with https://github.com/nodejs/node/blob/master/doc/STYLE_GUIDE.md for another PR (tools/doc
is not the first place I'd look for guidance on writing documentation)?
@richardlau I usually dare to do some cleansing in docs, but not so much of writing, as my English is clumsy. So this task may be better left for somebody from @nodejs/documentation with better language skills. I am just afraid to do major doc refactoring or create rather long text) |
tools/doc/README.md
Outdated
@@ -98,9 +101,9 @@ This event is emitted on instances of `SomeClass`, not on the module itself. | |||
``` | |||
|
|||
|
|||
* Classes have (description, Properties, Methods, Events). |
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.
Is there a reason behind this reordering?
The existing one is sorted in alphabetical order.
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 mean some logical hierarchy: from parent structures to child components mentioned in the previous lines. If this is not clear, I can revert this fragment,
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.
Classes and Functions go together, in most languages classes contain functions but in JavaScript everything is a function at the end.
Alphabetical order might be better.
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.
Reverted)
Alphabetical order restored. |
Landed in 9f6742d |
PR-URL: #20047 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: #20047 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: nodejs#20047 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: #20047 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes