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.

Sign up for GitHub

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

Jump to bottom

doc: explain _writev() API #31356

Closed
wants to merge 2 commits into from
Closed

doc: explain _writev() API #31356

wants to merge 2 commits into from

Conversation

HarshithaKP
Copy link
Member

the exact context of invocation of _writev API is not well known
also, the choice between _write and _writev is not well known.
add a description to make it explicit.

Fixes: #28408

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@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 Jan 14, 2020
@HarshithaKP
Copy link
Member Author

This is rework of stalled PR #28690.
cc: @tyof45

addaleax
addaleax approved these changes Jan 14, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good, and thanks for picking this up again!

I would suggest adding

Refs: https://github.com/nodejs/node/pull/28690
Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>

to the commit message in order to include attribution for the previous PR that this continues from.

doc/api/stream.md Outdated
multiple chunks of data at once. If implemented, the method will be called with
all chunks of data currently buffered in the write queue.
multiple chunks of data at once. If implemented and if there are buffered data
from previous writes, `_writev` (that is capable of handling chunks of data)
Copy link
Member

Choose a reason for hiding this comment

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

I think the previous sentence implies that _writev() can handle chunks of data, and there’s no need to duplicate that:

Suggested change
from previous writes, `_writev` (that is capable of handling chunks of data)
from previous writes, `_writev`

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax, Thanks. Fixed it.

@HarshithaKP @parkerbjur
doc: explain _writev() API
e2f3c96 
the exact context of invocation of _writev API is not well known
also, the choice between _write and _writev is not well known.
add a description to make it explicit.

Fixes: nodejs#28408
Refs: nodejs#28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>
Trott
Trott reviewed Jan 16, 2020
View reviewed changes
doc/api/stream.md Outdated
multiple chunks of data at once. If implemented, the method will be called with
all chunks of data currently buffered in the write queue.
multiple chunks of data at once. If implemented and if there are buffered data
from previous writes, `_writev` will be called instead of `_write`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
from previous writes, `_writev` will be called instead of `_write`.
from previous writes, `_writev()` will be called instead of `_write()`.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott, thanks. Fixed it.

Trott
Trott reviewed Jan 16, 2020
View reviewed changes
doc/api/stream.md Outdated
@@ -1886,8 +1886,8 @@ methods only.

The `writable._writev()` method may be implemented in addition or alternatively
to `writable._write()` in stream implementations that are capable of processing
multiple chunks of data at once. If implemented, the method will be called with
all chunks of data currently buffered in the write queue.
multiple chunks of data at once. If implemented and if there are buffered data
Copy link
Member

@Trott Trott Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: "are buffered data" is correct (because the word data is plural) but treating data as a singular noun has become idiomatic, is generally considered correct too, and seems to be what we do in the docs. Treating data as a singular word is so common that "data are" sounds jarring to many people.

I am unable to find any other examples of "data" + "are" in our docs, but many "data" + "is":

buffer.md: "the data is zero-filled"
http2.md: "incoming data is not being read"
net.md: "when the data is finally" and "when data is received"
stream.md: "Writable streams are an abstraction for a destination to which data is..." and "Readable streams are an abstraction for a source from which data is..."
vm.md: "data is produced successfully"

...and many other examples.

Therefore, for consistency with the rest of the docs, I recommend switching this to "is":

Suggested change
multiple chunks of data at once. If implemented and if there are buffered data
multiple chunks of data at once. If implemented and if there is buffered data

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott, done.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 16, 2020
@Trott
Copy link
Member

Trott commented Jan 16, 2020

Landed in e47a1eb

@Trott Trott closed this Jan 16, 2020
Trott pushed a commit that referenced this pull request Jan 16, 2020
@HarshithaKP @Trott
doc: explain _writev() API
e47a1eb 
The exact context of invocation of _writev API is not well known.
Also, the choice between _write and _writev is not well known.
Add a description to make it explicit.

Fixes: #28408
Refs: #28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>

PR-URL: #31356
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Feb 17, 2020
@HarshithaKP @codebytere
doc: explain _writev() API
bd54abe 
The exact context of invocation of _writev API is not well known.
Also, the choice between _write and _writev is not well known.
Add a description to make it explicit.

Fixes: #28408
Refs: #28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>

PR-URL: #31356
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 14, 2020
@HarshithaKP @codebytere
doc: explain _writev() API
2154a81 
The exact context of invocation of _writev API is not well known.
Also, the choice between _write and _writev is not well known.
Add a description to make it explicit.

Fixes: #28408
Refs: #28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>

PR-URL: #31356
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@HarshithaKP @codebytere
doc: explain _writev() API
ff8f0bc 
The exact context of invocation of _writev API is not well known.
Also, the choice between _write and _writev is not well known.
Add a description to make it explicit.

Fixes: #28408
Refs: #28690

Co-authored-by: Parker Bjur <bjur.parker45@gmail.com>

PR-URL: #31356
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
@HarshithaKP HarshithaKP mentioned this pull request Apr 1, 2020
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

writable stream _write and _writev
6 participants
@HarshithaKP @Trott @addaleax @lpinca @cjihrig @nodejs-github-bot