Skip to content

doc: writableFinished is true before 'finish' - #28811

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:fix-docs-writableFinished
Closed

doc: writableFinished is true before 'finish'#28811
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:fix-docs-writableFinished

Conversation

@ronag

@ronagronag commented Jul 23, 2019

Copy link
Copy Markdown
Member

The current description for stream.writableFinished is not entirely correct.

Checklist

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jul 23, 2019
@Trott

Trott commented Jul 23, 2019

Copy link
Copy Markdown
Member

Practically, I"m not sure what the difference is to the reader? Is this about what finished is set to when your finish event listener runs or something like that?

@ronag
ronagforce-pushed the fix-docs-writableFinished branch from 459e4ad to d3b065bCompareJuly 23, 2019 05:29
@ronag

ronag commented Jul 23, 2019

Copy link
Copy Markdown
MemberAuthor

It’s important in order to know whether when I register a finish listener if it will be called or not.

@ronag
ronagforce-pushed the fix-docs-writableFinished branch from d3b065b to 2502502CompareJuly 23, 2019 07:53
@ronagronag changed the title docs: writableFinished is true before 'finish'dos: writableFinished is true before 'finish'Jul 23, 2019
vsemozhetbyt
vsemozhetbyt reviewed Jul 23, 2019
View reviewed changes
Comment threaddoc/api/http.md Outdated
@ronag
ronagforce-pushed the fix-docs-writableFinished branch from 2502502 to cc13c69CompareJuly 23, 2019 10:16
sam-github
sam-github reviewed Jul 23, 2019
View reviewed changes
Comment threaddoc/api/http.md Outdated
@lpincalpinca changed the title dos: writableFinished is true before 'finish'doc: writableFinished is true before 'finish'Jul 23, 2019
@ronag
ronagforce-pushed the fix-docs-writableFinished branch 2 times, most recently from 5998562 to 0b677a6CompareJuly 23, 2019 20:50
@Trott

Trott commented Jul 25, 2019

Copy link
Copy Markdown
Member

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

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 25, 2019
@Trott

Trott commented Jul 26, 2019

Copy link
Copy Markdown
Member

@nodejs/streams

mcollina
mcollina requested changes Jul 26, 2019
View reviewed changes
Comment threaddoc/api/http.md Outdated
@trivikrtrivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 26, 2019
@ronag
ronagforce-pushed the fix-docs-writableFinished branch from 0b677a6 to c3b6f5bCompareAugust 2, 2019 06:34
@ronag
ronagforce-pushed the fix-docs-writableFinished branch from c3b6f5b to 204b76bCompareAugust 2, 2019 06:35
@Trott

Trott commented Aug 2, 2019

Copy link
Copy Markdown
Member

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

Trott
Trott reviewed Aug 2, 2019
View reviewed changes
Comment threaddoc/api/http.md
mcollina
mcollina approved these changes Aug 2, 2019
View reviewed changes

@mcollinamcollina 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

Trott pushed a commit to Trott/io.js that referenced this pull request Aug 2, 2019
@ronag@Trott
doc: writableFinished is true before 'finish'
0897782
PR-URL: nodejs#28811
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@Trott

Trott commented Aug 2, 2019

Copy link
Copy Markdown
Member

Landed in 0897782

@TrottTrott closed this Aug 2, 2019
targos pushed a commit that referenced this pull request Aug 2, 2019
@ronag@targos
doc: writableFinished is true before 'finish'
0ac6d28
PR-URL: #28811
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BridgeARBridgeAR mentioned this pull request Aug 6, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@mcollinamcollinamcollina approved these changes
+2 more reviewers
@sam-githubsam-githubsam-github left review comments
@vsemozhetbytvsemozhetbytvsemozhetbyt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docIssues and PRs related to the documentations.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ronag@Trott@sam-github@mcollina@vsemozhetbyt@trivikr@nodejs-github-bot