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

@@ -572,6 +572,12 @@ Examples of writable streams include:
 * [TCP sockets][]
 * [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

@@ -151,6 +151,10 @@ Stop watching for changes on the given `fs.FSWatcher`.

 Emitted when the ReadStream's file is opened.

+### 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

jennabelle commented May 1, 2016

mscdex May 1, 2016

jennabelle commented May 1, 2016

cjihrig May 1, 2016

jasnell May 1, 2016 •

edited

Loading

mscdex May 1, 2016

jasnell May 1, 2016

mscdex May 1, 2016

mcollina May 2, 2016

cjihrig commented May 1, 2016

jasnell commented May 1, 2016

jasnell commented May 1, 2016 •

edited

Loading

mcollina May 2, 2016

mcollina commented May 2, 2016

jennabelle commented May 2, 2016

mscdex commented May 2, 2016

jasnell commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

addaleax commented May 3, 2016

addaleax commented May 3, 2016 •

edited

Loading

jasnell commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

addaleax commented May 3, 2016

jasnell commented May 3, 2016

mcollina commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

mcollina commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

addaleax commented May 3, 2016


		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.

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

Conversation

jennabelle commented May 1, 2016

Checklist

Affected core subsystem(s)

Description of change

mscdex May 1, 2016

Choose a reason for hiding this comment

jennabelle commented May 1, 2016

cjihrig May 1, 2016

Choose a reason for hiding this comment

jasnell May 1, 2016 • edited Loading

Choose a reason for hiding this comment

mscdex May 1, 2016

Choose a reason for hiding this comment

jasnell May 1, 2016

Choose a reason for hiding this comment

mscdex May 1, 2016

Choose a reason for hiding this comment

mcollina May 2, 2016

Choose a reason for hiding this comment

cjihrig commented May 1, 2016

jasnell commented May 1, 2016

jasnell commented May 1, 2016 • edited Loading

mcollina May 2, 2016

Choose a reason for hiding this comment

mcollina commented May 2, 2016

jennabelle commented May 2, 2016

mscdex commented May 2, 2016

jasnell commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

addaleax commented May 3, 2016

addaleax commented May 3, 2016 • edited Loading

jasnell commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

addaleax commented May 3, 2016

jasnell commented May 3, 2016

mcollina commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

mcollina commented May 3, 2016

eljefedelrodeodeljefe commented May 3, 2016

addaleax commented May 3, 2016

jasnell May 1, 2016 •

edited

Loading

jasnell commented May 1, 2016 •

edited

Loading

addaleax commented May 3, 2016 •

edited

Loading