-
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: fix nits in stream.md #28591
doc: fix nits in stream.md #28591
Conversation
* Unify periods and upper case in comments. * Add missing parentheses for methods. * Add missing backticks. * Fix sorting position of `writable.writableFinished` section. * Replace a one-letter variable with a more readable one. * `catch(console.log)` -> `catch(console.error)`. * Document missing `emitClose` option in `new stream.Readable()` section mentioned in https://nodejs.org/api/stream.html#stream_event_close_1 and https://nodejs.org/api/stream.html#stream_readable_destroy_error copying from the `new stream.Writable()` section. Refs: https://github.com/nodejs/node/blob/36fdf1aa6c87ccfaebabb8f9c8004baab0549b0b/lib/_stream_readable.js#L121
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.
Not blocking. But I'd prefer a separate PR for the changes that only touch documentation. That will make it much easier if the code changes happen to introduce a subtle bug.
@fhinkel Sorry, I am not sure I understand. This PR only touches one doc file and no code files. How should I remake 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.
LGTM
Sorry, I've found out some more nits to fix in a separate fixup commit: a wrong link format with an empty href, missing return value and some missing parentheses for methods. |
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.
Still LGTM
Landed in 1cc5c54 |
* Unify periods and upper case in comments. * Add missing parentheses for methods. * Add missing backticks. * Fix sorting position of `writable.writableFinished` section. * Replace a one-letter variable with a more readable one. * `catch(console.log)` -> `catch(console.error)`. * Document missing `emitClose` option in `new stream.Readable()` section mentioned in https://nodejs.org/api/stream.html#stream_event_close_1 and https://nodejs.org/api/stream.html#stream_readable_destroy_error copying from the `new stream.Writable()` section. Refs: https://github.com/nodejs/node/blob/36fdf1aa6c87ccfaebabb8f9c8004baab0549b0b/lib/_stream_readable.js#L121 PR-URL: nodejs#28591 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
* Unify periods and upper case in comments. * Add missing parentheses for methods. * Add missing backticks. * Fix sorting position of `writable.writableFinished` section. * Replace a one-letter variable with a more readable one. * `catch(console.log)` -> `catch(console.error)`. * Document missing `emitClose` option in `new stream.Readable()` section mentioned in https://nodejs.org/api/stream.html#stream_event_close_1 and https://nodejs.org/api/stream.html#stream_readable_destroy_error copying from the `new stream.Writable()` section. Refs: https://github.com/nodejs/node/blob/36fdf1aa6c87ccfaebabb8f9c8004baab0549b0b/lib/_stream_readable.js#L121 PR-URL: #28591 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passeswritable.writableFinished
section.catch(console.log)
->catch(console.error)
.emitClose
option innew stream.Readable()
section mentioned instream.Readable
Event: 'close'
andreadable.destroy([error])
sections copying from thenew stream.Writable()
section. Refs:lib/_stream_readable.js#L121
.