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: stream.finished cleanup #28935

Closed
wants to merge 2 commits into from
Closed

doc: stream.finished cleanup #28935

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 2, 2019

Documents stream.finished cleanup behaviour.

Checklist

@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 Aug 2, 2019
@ronag ronag mentioned this pull request Aug 2, 2019
Closed
4 tasks
@ronag ronag changed the title docs: stream.finished cleanup doc: stream.finished cleanup Aug 2, 2019
@ronag ronag force-pushed the doc-finished-cleanup branch 3 times, most recently from db172c7 to 12448b1 Compare August 2, 2019 07:04
mcollina
mcollina reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@ronag ronag force-pushed the doc-finished-cleanup branch 2 times, most recently from 7b2d4af to 07f5751 Compare August 2, 2019 07:14
vsemozhetbyt
vsemozhetbyt reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
vsemozhetbyt
vsemozhetbyt reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@vsemozhetbyt
Copy link
Contributor

Just a nit: it seems "note that" is avoided in the docs recently, see #28329, #28327, #21646

@ronag ronag force-pushed the doc-finished-cleanup branch 2 times, most recently from 4552345 to f049f17 Compare August 2, 2019 17:01
Trott
Trott reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Aug 2, 2019
View reviewed changes
doc/api/stream.md Outdated
### stream.pipeline(...streams, callback)

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated whitespace change?

Copy link
Member

Choose a reason for hiding this comment

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

@ronag ^^

Trott
Trott approved these changes Aug 2, 2019
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.

LGTM assuming nits/suggestions are applied.

@ronag ronag force-pushed the doc-finished-cleanup branch 2 times, most recently from afd8eff to 264bb84 Compare August 3, 2019 10:15
This was referenced Aug 6, 2019
Closed
Closed
@Trott
Copy link
Member

Trott commented Aug 20, 2019

Rebase to remove conflict?

@Trott
Copy link
Member

Trott commented Aug 20, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3838/

@Trott Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 20, 2019
@ronag
Copy link
Member Author

ronag commented Aug 23, 2019

Fixed travis lint failure.

@ronag
Copy link
Member Author

ronag commented Sep 18, 2019

@Trott: Is this missing anything?

@Trott
Copy link
Member

Trott commented Sep 22, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3919/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 22, 2019
Trott pushed a commit that referenced this pull request Sep 22, 2019
@ronag @Trott
doc: explain stream.finished cleanup
695db97 
PR-URL: #28935
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Sep 22, 2019 •
edited
Loading

Landed in 4c40a64

@Trott Trott closed this Sep 22, 2019
Trott pushed a commit that referenced this pull request Sep 22, 2019
@ronag @Trott
doc: explain stream.finished cleanup
4c40a64 
PR-URL: #28935
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Sep 23, 2019
@ronag @targos
doc: explain stream.finished cleanup
118b8bf 
PR-URL: #28935
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
BridgeAR pushed a commit that referenced this pull request Sep 25, 2019
@ronag @BridgeAR
doc: explain stream.finished cleanup
3ba6464 
PR-URL: #28935
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Sep 25, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@Trott Trott Trott approved these changes

@lpinca lpinca lpinca 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.
6 participants
@ronag @vsemozhetbyt @Trott @mcollina @lpinca @nodejs-github-bot