Skip to content

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

Closed
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:require-end-of-stream-callback
Closed

stream,doc: require finished callback and add options docs#21058
BridgeAR wants to merge 3 commits into
nodejs:masterfrom
BridgeAR:require-end-of-stream-callback

Conversation

@BridgeAR

@BridgeAR BridgeAR commented May 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

@Trott Trott left a comment

Copy link
Copy Markdown
Member

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.

Comment thread doc/api/stream.md Outdated

@Trott Trott May 31, 2018

Copy link
Copy Markdown
Member

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 ...

@Trott Trott May 31, 2018

Copy link
Copy Markdown
Member

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)

Comment thread doc/api/stream.md Outdated

@Trott Trott May 31, 2018

Copy link
Copy Markdown
Member

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?

Comment thread doc/api/stream.md Outdated

@Trott Trott May 31, 2018

Copy link
Copy Markdown
Member

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.

Comment thread doc/api/stream.md Outdated

@Trott Trott May 31, 2018

Copy link
Copy Markdown
Member

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.

Comment thread doc/api/stream.md Outdated

@Trott Trott May 31, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ended -> ends

Comment thread doc/api/stream.md Outdated

@Trott Trott May 31, 2018

Copy link
Copy Markdown
Member

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

vsemozhetbyt commented Jun 2, 2018

Copy link
Copy Markdown
Contributor

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 Node.js documentation. stream Issues and PRs related to Node.js streams. labels Jun 2, 2018
@mafintosh

mafintosh commented Jun 5, 2018

Copy link
Copy Markdown
Member

LGTM on the changes mentioned from me

mcollina
mcollina approved these changes Aug 20, 2018
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

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

BridgeAR commented Aug 20, 2018

Copy link
Copy Markdown
Member Author

@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

mcollina commented Aug 21, 2018

Copy link
Copy Markdown
Member

Still LGTM, this can land.

@mcollina mcollina added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Aug 21, 2018
@BridgeAR

BridgeAR commented Aug 21, 2018

Copy link
Copy Markdown
Member Author

@nodejs/tsc PTAL

@mafintosh

mafintosh commented Aug 21, 2018

Copy link
Copy Markdown
Member

@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

BridgeAR commented Aug 21, 2018

Copy link
Copy Markdown
Member Author

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 with CI started, the required approvals, and no outstanding review comments. doc Issues and PRs related to Node.js documentation. semver-major PRs that contain breaking changes and should be released in the next major version. stream Issues and PRs related to Node.js streams.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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