Skip to content

stream,doc: require finished callback and add options docs #21058

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
Closed
BridgeAR wants to merge 3 commits into from
Closed

stream,doc: require finished callback and add options docs #21058

BridgeAR wants to merge 3 commits into from

Conversation

@BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 31, 2018 •
edited
Loading

I just stumbled upon missing documentation about the Stream.finished() options. We should also make the callback mandatory as already documented. Right now there would be a no op added in case a falsy value is provided for the callback.

Test cases are missing and I'll add some later on (if someone wants to add them, I am also more than happy about that ;-) please feel free to just directly pushing on this branch).

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

@BridgeAR BridgeAR added wip Issues and PRs that are still a work in progress. semver-major PRs that contain breaking changes and should be released in the next major version. labels May 31, 2018
Trott
Trott previously requested changes May 31, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Content LGTM. Some formatting/grammar changes requested.

doc/api/stream.md Outdated
Copy link
Member

@Trott Trott May 31, 2018

Choose a reason for hiding this comment

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

Comma after `false`, and maybe then as well:

If set to `false`, then a call to ...

Copy link
Member

@Trott Trott May 31, 2018

Choose a reason for hiding this comment

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

Backticks around emit('error', err)

doc/api/stream.md Outdated
Copy link
Member

@Trott Trott May 31, 2018

Choose a reason for hiding this comment

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

Missing word or something. treated as finished perhaps?

doc/api/stream.md Outdated
Copy link
Member

@Trott Trott May 31, 2018

Choose a reason for hiding this comment

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

Same thing with the comma and probably a then as well.

doc/api/stream.md Outdated
Copy link
Member

@Trott Trott May 31, 2018

Choose a reason for hiding this comment

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

Same thing with the comma and probably a then as well.

doc/api/stream.md Outdated
Copy link
Member

@Trott Trott May 31, 2018

Choose a reason for hiding this comment

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

ended -> ends

doc/api/stream.md Outdated
Copy link
Member

@Trott Trott May 31, 2018

Choose a reason for hiding this comment

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

ended -> ends

@nodejs nodejs deleted a comment from refack Jun 1, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jun 2, 2018

When we change a heading in the docs, we need to check if there are links to this heading in all the docs (copy the old hash from the old HTML doc and grep this hash in *.md sources). Currently, this one link in stream.md also need to be updated:

https://github.com/nodejs/node/blame/master/doc/api/stream.md#L2504

@vsemozhetbyt vsemozhetbyt added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jun 2, 2018
@mafintosh
Copy link
Member

mafintosh commented Jun 5, 2018

LGTM on the changes mentioned from me

mcollina
mcollina approved these changes Aug 20, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR
lib: require a callback for end-of-stream
3d893b9 
Make the callback mandatory as mostly done in all other Node.js
callback APIs so users explicitly have to decide what to do in error
cases.
@BridgeAR
doc: add options documentation to Stream.finished()
48b32f9 
When originally implemented it was missed that Stream.finished() has
an optional options object. This adds the docs to those.
@BridgeAR
fixup: address comments & add tests
c310236
@BridgeAR BridgeAR force-pushed the require-end-of-stream-callback branch from 27c3883 to c310236 Compare August 20, 2018 23:27
@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label Aug 20, 2018
@BridgeAR BridgeAR requested a review from a team August 20, 2018 23:38
@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 20, 2018

@Trott @nodejs/streams PTAL

I addressed the comments and added some tests.

CI https://ci.nodejs.org/job/node-test-pull-request/16622/

@Trott Trott dismissed their stale review August 21, 2018 05:02

formatting/grammar fixed

@mcollina
Copy link
Member

mcollina commented Aug 21, 2018

Still LGTM, this can land.

@mcollina mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 21, 2018
@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 21, 2018

@nodejs/tsc PTAL

@mafintosh
Copy link
Member

mafintosh commented Aug 21, 2018

@BridgeAR still lgtm

BridgeAR added a commit to BridgeAR/node that referenced this pull request Aug 21, 2018
@BridgeAR
lib: require a callback for end-of-stream
36468ca 
Make the callback mandatory as mostly done in all other Node.js
callback APIs so users explicitly have to decide what to do in error
cases.

This also documents the options for `Stream.finished()`.

When originally implemented it was missed that Stream.finished() has
an optional options object.

PR-URL: nodejs#21058
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mathias Buus <mathiasbuus@gmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented Aug 21, 2018

Landed in 36468ca 🎉

@BridgeAR BridgeAR closed this Aug 21, 2018
@BridgeAR BridgeAR deleted the require-end-of-stream-callback branch January 20, 2020 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott left review comments

@mafintosh mafintosh Awaiting requested review from mafintosh

@addaleax addaleax Awaiting requested review from addaleax

@Fishrock123 Fishrock123 Awaiting requested review from Fishrock123

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. semver-major PRs that contain breaking changes and should be released in the next major version. 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.

6 participants

@BridgeAR @vsemozhetbyt @mafintosh @mcollina @jasnell @Trott