-
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
Fix fs & util docs #18775
Fix fs & util docs #18775
Conversation
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
doc/api/fs.md
Outdated
@@ -1210,6 +1210,7 @@ changes: | |||
* `start` {integer} | |||
* `end` {integer} | |||
* `highWaterMark` {integer} | |||
* Returns: {ReadStream} |
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.
Should be {stream.Readable}
See type-parser.js#L81-L84
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.
Thank you for your kind pointing out!
doc/api/fs.md
Outdated
@@ -1285,6 +1286,7 @@ changes: | |||
* `mode` {integer} | |||
* `autoClose` {boolean} | |||
* `start` {integer} | |||
* Returns: {WriteStream} |
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.
Should be {stream.Writable}
See type-parser.js#L81-L84
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.
Thanks for this, going to block so this doesn't land with ReadStream and WriteStream by mistake - looks good other than that
@benjamingr @vsemozhetbyt I added a fixing commit about stream type name! Please confirm it 🙇 |
By the way, I wonder that specifying like |
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, thanks for this
Yes, I think it makes sense. |
@lpinca Thanks! Then I'm gonna remove them. But there are 12 |
@shqld It's probably better to use a dedicated PR. |
I see! 😄 |
PR-URL: nodejs#18775 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
PR-URL: nodejs#18775 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
Should this be backported to |
I think it should. I'll do that. |
Backport-PR-URL: #19127 PR-URL: #18775 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
Backport-PR-URL: #19127 PR-URL: #18775 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
PR-URL: nodejs#18775 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
PR-URL: nodejs#18775 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
Overview
util.isDeepStrictEqual
ofdoc/api/util.md
.doc/api/
fs.md
&util.md
.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
doc