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: update stream.md #9468

Closed
wants to merge 3 commits into from
Closed

doc: update stream.md #9468

wants to merge 3 commits into from

Conversation

tanujasawant
Copy link
Contributor

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

stream.md is updated to explain the return value of
writable.write(chunk) precisely.
Fixes: #9247

stream.md is updated to explain the return value of
writable.write(chunk) precisely.
Fixes: #9247
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Nov 4, 2016
The return value is `true` if the internal buffer does not exceed
`highWaterMark` configured when the stream was created after admitting
`chunk`. If false is returned, further attempts to write data to the stream
should be paused until the [`'drain'`][] event is emitted. However, the
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest: "should be paused" -> "should stop"

I find the use of the word paused confusing, because there is a stream.pause() method, but calling it is not the "paused" referred to in the documentation.

should be paused until the [`'drain'`][] event is emitted.
The return value is `true` if the internal buffer does not exceed
`highWaterMark` configured when the stream was created after admitting
`chunk`. If false is returned, further attempts to write data to the stream
Copy link
Contributor

Choose a reason for hiding this comment

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

false -> false

stream.md is updated to explain the return value of
writable.write(chunk) precisely.
Fixes: #9247
@tanujasawant
Copy link
Contributor Author

@sam-github @cjihrig please check, thanks for the feedback :)

`chunk`. If `false` is returned, further attempts to write data to the stream
should stop until the [`'drain'`][] event is emitted. However, the
`false` return value is only advisory and the writable stream will
unconditionally accept `chunk` even if it has not not been allowed to drain.
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: accept -> accept and buffer

stream.md is updated to explain the return value of
writable.write(chunk) precisely.
Fixes: #9247
@tanujasawant
Copy link
Contributor Author

@ronkorving please check now, thanks!

@silverwind
Copy link
Contributor

ping @ronkorving.

Copy link
Contributor

@binki binki left a comment

Choose a reason for hiding this comment

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

“internal buffer does not exceed highWaterMark” needs to convey the meaning “internal buffer is smaller than highWaterMark”.

@ronkorving
Copy link
Contributor

Apologies, my GitHub notifications are starting to look like my inbox. LGTM!

@silverwind
Copy link
Contributor

Thanks! Landed with wrapping and commit message fixed in f347dad.

@silverwind silverwind closed this Dec 21, 2016
silverwind pushed a commit that referenced this pull request Dec 21, 2016
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
stream.md is updated to explain the return value of
writable.write(chunk) precisely.

PR-URL: #9468
Fixes: #9247
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ron Korving <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
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.

streams documentation for writable.write(chunk) describes return value incorrectly
9 participants