Skip to content
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: multiple improvements in Stream docs #5009

Closed
wants to merge 1 commit into from
Closed

doc: multiple improvements in Stream docs #5009

wants to merge 1 commit into from

Conversation

estliberitas
Copy link
Contributor

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

@targos 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
returns false, at which point it should stop reading from the resource. Only
when \_read is called again after it has stopped should it start reading
more data from the resource and pushing that data onto the queue.
When `_read()` is called, if data is available from the resource, this method
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@chrisdickinson
Copy link
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!

is consumed.

Buffering in Writable streams happens when the user calls
[`stream.write(chunk)`][] repeatedly, even when `write()` returns `false`.
[`stream.write(chunk)`][] repeatedly, even when [`write()`][] returns `false`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we simply say 'it returns'?

@estliberitas
Copy link
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?

@thefourtheye
Copy link
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?

@estliberitas
Copy link
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.

@estliberitas
Copy link
Contributor Author

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

@thefourtheye
Copy link
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]

@estliberitas
Copy link
Contributor Author

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

@estliberitas
Copy link
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

@thefourtheye
Copy link
Contributor

Sweeeet :-)

@estliberitas
Copy link
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
Copy link
Member

Qard commented Feb 2, 2016

LGTM

@thefourtheye
Copy link
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 :-)

@thefourtheye
Copy link
Contributor

LGTM

@estliberitas
Copy link
Contributor Author

@thefourtheye will do ;)

@jasnell
Copy link
Member

jasnell commented Feb 2, 2016

@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!

@estliberitas
Copy link
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.
@estliberitas
Copy link
Contributor Author

@jasnell done

@jasnell
Copy link
Member

jasnell commented Feb 2, 2016

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
Copy link
Member

jasnell commented Feb 2, 2016

Landed in b0b4aeb

@jasnell 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 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>
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