Documentation/Add 'close' events to fs.ReadStream, fs.WriteStream, etc per Issue #6484#6499

jennabelle · 2016-05-01T08:14:35Z

Checklist

[ x ] documentation is changed or added
[ x ] the commit message follows commit guidelines

Affected core subsystem(s)

Description of change

Per Issue #6484

Add 'close' event to doc/api/fs.md --> fs.ReadStream
Add 'close' event to doc/api/fs.md --> fs.WriteStream
Add 'close event to doc/api/stream.md --> stream.Writable

mscdex · 2016-05-01T08:19:53Z

doc/api/stream.md

 *[child process stdin][]
 *[`process.stdout`][], [`process.stderr`][]
+
+Event: 'close'


This needs to be prefixed with ####.

… stream.Readable event: 'close' section

…ream

jennabelle · 2016-05-01T08:36:53Z

@mscdex made those changes you requested let me know if anything further is needed

cjihrig · 2016-05-01T13:23:34Z

doc/api/fs.md


+### Event: 'close'
+
+Emitted when the ReadStream's underlying file descriptor has been closed. Comes from fs.close() method. 


Can you add a "the" in front of fs.close(). Also, please put fs.close() in backticks.

Suggested rewording (note also the addition of the backticks )
Emitted when the `ReadStream`'s underlying file descriptor has been closed using `fs.close()`.
(same below for WriteStream also)

Is the note about fs.close() really that helpful/useful? Maybe we could just drop that part entirely?

I'm good with it either way

The reason I ask is because either way we're wording it makes it kind of sound like the end user is expected to call fs.close()/stream.close() to see this event.

I do not think 'close' is emitted only with fs.close(), or am I wrong?

cjihrig · 2016-05-01T13:25:16Z

Can you wrap long lines at 80 characters.

jasnell · 2016-05-01T13:58:23Z

LGTM with nits addressed

jasnell · 2016-05-01T13:58:35Z

@nodejs/documentation @nodejs/streams

mcollina · 2016-05-02T06:26:23Z

doc/api/stream.md

+
+Emitted when the stream and any of its underlying resources (a file descriptor, for example) have been closed. The event indicates that no more events will be emitted, and no further computation will occur.
+
+Not all streams will emit the `'close'` event.


I will say this is an optional event for implementors.

mcollina · 2016-05-02T06:40:21Z

Good work @jennabelle! We need more of these, keep them coming. All the streams API needs a lot of help anyway, and we need to tackle the "big" challenge of documenting the whole state machine. If you like, join us in our monthly meeting: nodejs/readable-stream#196.

per Issue #6484

per issue #6484

jennabelle · 2016-05-02T18:00:03Z

@mcollina Thank you! I am a new contributor so any tips and feedback are welcome. Looking forward to collaborating more!

mscdex · 2016-05-02T18:10:11Z

LGTM

jasnell · 2016-05-03T16:43:41Z

Is this ready to land?

eljefedelrodeodeljefe · 2016-05-03T16:54:43Z

Sorry to be so nitty but the last line is 81 chars. Otherwise LGTM.

eljefedelrodeodeljefe · 2016-05-03T16:55:10Z

Thanks for the effort @jennabelle

addaleax · 2016-05-03T17:24:05Z

LGTM with wrapped lines, too. Thanks!

addaleax · 2016-05-03T17:28:12Z

Btw, the .close() method itself is currently undocumented, but I guess that doesn’t have to happen here. And ping @Trott because he talked about that in irc.

jasnell · 2016-05-03T17:55:11Z

@eljefedelrodeodeljefe @addaleax ... that can be fixed quickly by whomever lands the PR :-)

eljefedelrodeodeljefe · 2016-05-03T18:04:57Z

Didn't know that we are allowed and ecouraged to edit contents except commit messages while landing. Good to know. Then let's do it.

addaleax · 2016-05-03T18:08:28Z

@eljefedelrodeodeljefe Care to do the honours? 😉

jasnell · 2016-05-03T18:10:33Z

It's generally preferable to have the author of the PR edit the nits overall but for extremely minor stuff fixing on landing is fine, especially when they're just linting or linewrap issues. Just make sure you squash the additional commit and maintain the original author.

Add 'close' event to doc/api/fs.md --> fs.ReadStream Add 'close' event to doc/api/fs.md --> fs.WriteStream Add 'close event to doc/api/stream.md --> stream.Writable From squashed history: Add 'close' event to stream.Writable per Issue #6484 Add #### prefix to Event: 'close' and backticks to 'close' similar to stream.Readable event: 'close' section Add more specifics to 'close' events for fs.ReadStream and fs.WriteStream Fix/Changed 'close' event from 'fs.ReadStream' to 'fs.WriteStream' wrapped long lines at 80 chars, reworded per Issue #6484 including the 'close' event as optional add 'close' event as optional in stream.Readable per issue #6484 doc: Add 'close' events to fs.ReadStream, 80char nit Fixes: #6484 PR-URL: #6499 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>

mcollina · 2016-05-03T18:37:49Z

Can we get a couple more LGTMs from @nodejs/streams?

@mafintosh can you have a look?

eljefedelrodeodeljefe · 2016-05-03T18:38:14Z

Thanks. Landed in 2aa3769.

mcollina · 2016-05-03T18:38:31Z

@mafintosh can you have a look anyway? :)

eljefedelrodeodeljefe · 2016-05-03T18:40:00Z

Yeh, sorry :) Give me a ping if somethings wrong @mafintosh

addaleax · 2016-05-03T19:38:06Z

@eljefedelrodeodeljefe Btw, I think the Fixes: field should contain the full issue URL, too. :)

Add 'close' event to doc/api/fs.md --> fs.ReadStream Add 'close' event to doc/api/fs.md --> fs.WriteStream Add 'close event to doc/api/stream.md --> stream.Writable From squashed history: Add 'close' event to stream.Writable per Issue #6484 Add #### prefix to Event: 'close' and backticks to 'close' similar to stream.Readable event: 'close' section Add more specifics to 'close' events for fs.ReadStream and fs.WriteStream Fix/Changed 'close' event from 'fs.ReadStream' to 'fs.WriteStream' wrapped long lines at 80 chars, reworded per Issue #6484 including the 'close' event as optional add 'close' event as optional in stream.Readable per issue #6484 doc: Add 'close' events to fs.ReadStream, 80char nit Fixes: #6484 PR-URL: #6499 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>

Add 'close' event to doc/api/fs.md --> fs.ReadStream Add 'close' event to doc/api/fs.md --> fs.WriteStream Add 'close event to doc/api/stream.md --> stream.Writable From squashed history: Add 'close' event to stream.Writable per Issue nodejs#6484 Add #### prefix to Event: 'close' and backticks to 'close' similar to stream.Readable event: 'close' section Add more specifics to 'close' events for fs.ReadStream and fs.WriteStream Fix/Changed 'close' event from 'fs.ReadStream' to 'fs.WriteStream' wrapped long lines at 80 chars, reworded per Issue nodejs#6484 including the 'close' event as optional add 'close' event as optional in stream.Readable per issue nodejs#6484 doc: Add 'close' events to fs.ReadStream, 80char nit Fixes: nodejs#6484 PR-URL: nodejs#6499 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>

Add 'close' event to doc/api/fs.md --> fs.ReadStream Add 'close' event to doc/api/fs.md --> fs.WriteStream Add 'close event to doc/api/stream.md --> stream.Writable From squashed history: Add 'close' event to stream.Writable per Issue #6484 Add #### prefix to Event: 'close' and backticks to 'close' similar to stream.Readable event: 'close' section Add more specifics to 'close' events for fs.ReadStream and fs.WriteStream Fix/Changed 'close' event from 'fs.ReadStream' to 'fs.WriteStream' wrapped long lines at 80 chars, reworded per Issue #6484 including the 'close' event as optional add 'close' event as optional in stream.Readable per issue #6484 doc: Add 'close' events to fs.ReadStream, 80char nit Fixes: #6484 PR-URL: #6499 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Robert Jefe Lindstaedt <[email protected]>

Jenna Vuong added 2 commits May 1, 2016 00:58

Add 'close' events to fs.ReadStream and fs.WriteSTream
9d48b5d

Add 'close' event to stream.Writable per Issue#6484
ff217a8

nodejs-github-bot added the doc Issues and PRs related to the documentations. label May 1, 2016

mscdex added fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem. labels May 1, 2016

mscdex reviewed May 1, 2016
View reviewed changes

Jenna Vuong added 3 commits May 1, 2016 01:28

Add #### prefix to Event: 'close' and backticks to 'close' similar to…
827dfcb
… stream.Readable event: 'close' section

Add more specifics to 'close' events for fs.ReadStream and fs.WriteSt…
57aa9e6
…ream

Fix/Changed 'close' event from 'fs.ReadStream' to 'fs.WriteStream'
439df4b

cjihrig reviewed May 1, 2016
View reviewed changes

mcollina reviewed May 2, 2016
View reviewed changes

Jenna Vuong added 3 commits May 2, 2016 10:21

wrapped long lines at 80 chars, reworded
b2a0db1
per Issue #6484

including the 'close' event as optional
bcc8bd2

add 'close' event as optional in stream.Readable
8cdf720
per issue #6484

eljefedelrodeodeljefe closed this May 3, 2016

This was referenced May 4, 2016
Propose v6.1.0 wrong base please ignore #6556
Closed
Propose v6.1.0 (Openssl update) #6557
Merged

MylesBorins added the dont-land-on-v4.x label Jun 2, 2016

addaleax mentioned this pull request Jul 12, 2016
Backport fs added: info #7679
Closed

MylesBorins mentioned this pull request Jul 12, 2016
v4.5.0 proposal #7688
Merged


		### Event: 'close'

		Emitted when the ReadStream's underlying file descriptor has been closed. Comes from fs.close() method.


		Emitted when the stream and any of its underlying resources (a file descriptor, for example) have been closed. The event indicates that no more events will be emitted, and no further computation will occur.

		Not all streams will emit the `'close'` event.

Uh oh!

Documentation/Add 'close' events to fs.ReadStream, fs.WriteStream, etc per Issue #6484#6499

Documentation/Add 'close' events to fs.ReadStream, fs.WriteStream, etc per Issue #6484 #6499

Uh oh!

Conversation

jennabelle commented May 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

mscdexMay 1, 2016

Choose a reason for hiding this comment

Uh oh!

jennabelle commented May 1, 2016

Uh oh!

cjihrigMay 1, 2016

Choose a reason for hiding this comment

Uh oh!

jasnellMay 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdexMay 1, 2016

Choose a reason for hiding this comment

Uh oh!

jasnellMay 1, 2016

Choose a reason for hiding this comment

Uh oh!

mscdexMay 1, 2016

Choose a reason for hiding this comment

Uh oh!

mcollinaMay 2, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig commented May 1, 2016

Uh oh!

jasnell commented May 1, 2016

Uh oh!

jasnell commented May 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollinaMay 2, 2016

Choose a reason for hiding this comment

Uh oh!

mcollina commented May 2, 2016

Uh oh!

jennabelle commented May 2, 2016

Uh oh!

mscdex commented May 2, 2016

Uh oh!

jasnell commented May 3, 2016

Uh oh!

eljefedelrodeodeljefe commented May 3, 2016

Uh oh!

eljefedelrodeodeljefe commented May 3, 2016

Uh oh!

addaleax commented May 3, 2016

Uh oh!

addaleax commented May 3, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 3, 2016

Uh oh!

eljefedelrodeodeljefe commented May 3, 2016

Uh oh!

addaleax commented May 3, 2016

Uh oh!

jasnell commented May 3, 2016

Uh oh!

mcollina commented May 3, 2016

Uh oh!

eljefedelrodeodeljefe commented May 3, 2016

Uh oh!

mcollina commented May 3, 2016

Uh oh!

eljefedelrodeodeljefe commented May 3, 2016

Uh oh!

addaleax commented May 3, 2016

Uh oh!

Reviewers

jasnellMay 1, 2016•
edited
Loading

jasnell commented May 1, 2016•
edited
Loading

addaleax commented May 3, 2016•
edited
Loading