FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: multiple improvements in Stream docs by estliberitas · Pull Request #5009 · nodejs/node · GitHub

/ node Public

doc: multiple improvements in Stream docs - #5009

Closed
estliberitas wants to merge 1 commit into
nodejs:masterfrom
estliberitas:doc-stream-fixes
Closed

doc: multiple improvements in Stream docs#5009
estliberitas wants to merge 1 commit into
nodejs:masterfrom
estliberitas:doc-stream-fixes

Conversation

Copy link
Copy Markdown
Contributor

Add links, fix constants and functions styling. Minor lexical corrections.

targos added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. lts-watch-v4.x labels Jan 31, 2016
Comment thread doc/api/stream.markdown Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you change this method to the _read() implementation?

Copy link
Copy Markdown
Contributor

Great work! Once the wording nit is corrected, this LGTM. We usually leave PRs open once they've been LGTM'd for about 48 hours to make sure all of the contributors get a chance to review. If no one has objections it should be merged shortly after that. Thanks for the contribution!

Comment thread doc/api/stream.markdown Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can't we simply say 'it returns'?

Copy link
Copy Markdown
Contributor Author

@thefourtheye BTW, I was wondering. In this file there are a lot of duplicates and in others which I worked on too. Maybe replace those where possible by some default except places where arguments matter to the context of sentences (like "for this case use read(0)"). If so, what would be default stream.write(), write() or smth. different? I find stream.write() more appropriate because it equals to name of header where this method is described
P.S. would be cool to create some kind of doc style guide or it already exists?

Copy link
Copy Markdown
Contributor

@estliberitas Yes, I was about to talk to you about the same (I couldn't earlier because I was on Mobile). What we probably can do is, use stream.write and stream.read consistently. And whenever we see the call with specific arguments, we can say something like "for this case use [stream.read()][] with 0". May be we can add a small description about the parameter being passed. What do you think?

Copy link
Copy Markdown
Contributor Author

@thefourtheye Well, I find it a good idea. And as for multiple arguments, we just separate by comma? Like, for example, lines 1582-1583:

A Writable stream in object mode will always ignore the encoding
argument to stream.write() when called with chunk, encoding.

To me it's not so cool, but at least it's informative and saves us from copy-pasting a lot.

Other concern is optional arguments:

Note that calling stream.read() with [size] after the 'end' event has
been triggered will return null. No runtime error will be raised.

Copy link
Copy Markdown
Contributor Author

@thefourtheye Other idea would be patch doc tool to look for links without arguments 😉

Copy link
Copy Markdown
Contributor

Or we can use a common name like this [stream.read(0)][stream-read] and then we can tag all the links with various arguments with [stream-read]

Copy link
Copy Markdown
Contributor Author

@thefourtheye Wow, I got something new today. Thanks, I think that resolves all the questions above.

Copy link
Copy Markdown
Contributor Author

@chrisdickinson @thefourtheye Guys, please check once have a time.

To simplify life I made a small script to check links consistency: https://gist.github.com/estliberitas/bfd35c7a5a977aa5d221

Copy link
Copy Markdown
Contributor

Sweeeet :-)

Copy link
Copy Markdown
Contributor Author

Btw, seems I did same thing as in #5003 and #5007

Also, I used that script from Gist above for other files, and it seems there are things to do yet for other API doc files.

Qard commented Feb 2, 2016

Copy link
Copy Markdown
Member

LGTM

Copy link
Copy Markdown
Contributor

it seems there are things to do yet for other API doc files

Please go ahead and submit PRs. I always wanted to clean this thing myself. Glad you are doing this :-)

Copy link
Copy Markdown
Contributor

LGTM

Copy link
Copy Markdown
Contributor Author

@thefourtheye will do ;)

jasnell commented Feb 2, 2016

Copy link
Copy Markdown
Member

@estliberitas ... there have been some other changes that touch on the same file, can I ask you to rebase this and update so we can remove the duplication? Thank you!

Copy link
Copy Markdown
Contributor Author

Yep, I guess it's #5007, ok will do now.

Add missing links, remove duplicate ones, fix constants and functions styling.
Minor lexical corrections.

Copy link
Copy Markdown
Contributor Author

@jasnell done

jasnell commented Feb 2, 2016

Copy link
Copy Markdown
Member

LGTM. Thank you!

jasnell pushed a commit that referenced this pull request Feb 2, 2016
Add missing links, remove duplicate ones, fix constants and functions styling.
Minor lexical corrections.

PR-URL: #5009
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Feb 2, 2016

Copy link
Copy Markdown
Member

Landed in b0b4aeb

jasnell closed this Feb 2, 2016
rvagg pushed a commit that referenced this pull request Feb 8, 2016
Add missing links, remove duplicate ones, fix constants and functions styling.
Minor lexical corrections.

PR-URL: #5009
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
estliberitas deleted the doc-stream-fixes branch February 14, 2016 17:05
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
Add missing links, remove duplicate ones, fix constants and functions styling.
Minor lexical corrections.

PR-URL: #5009
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 22, 2016
Add missing links, remove duplicate ones, fix constants and functions styling.
Minor lexical corrections.

PR-URL: #5009
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Add missing links, remove duplicate ones, fix constants and functions styling.
Minor lexical corrections.

PR-URL: #5009
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Add missing links, remove duplicate ones, fix constants and functions styling.
Minor lexical corrections.

PR-URL: nodejs#5009
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL