Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

doc: several documentation improvements #25635

Closed
wants to merge 7 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 6, 2015

bundled together to make landing them easier.

bsteephenson and others added 5 commits June 29, 2015 12:13
Clarifies that emitter.listener() returns a copy, not a reference
Resolves issue nodejs#9022
per: nodejs#14596

1. document that a runtime error will occur if you attempt
   to unshift after the end event
2. document that calling read after the end event will return
   null and will not trigger a runtime error
Minor clarifications around Readable._read and Readable.push
to make their implementation/usage easier to understand.

nodejs#14124 (comment)
Fix for nodejs#25559 (Typo in example of util.deprecate() documentation)
Per nodejs#14604,

Document that performing an `unshift` during a read
can have unexpected results. Following the `unshift`
with a `push('')` resets the reading state appropriately.
Also indicate that doing an `unshift` during a read
is not optimal and should be avoided.
@jasnell
Copy link
Member Author

jasnell commented Jul 6, 2015

@joyent/node-collaborators

var fs = require('fs');
var rr = fs.createReadStream('foo.txt');
rr.on('readable', function() {
var chunk;

Choose a reason for hiding this comment

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

Is this variable only for demonstration? Not being used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah...right, just mentally zoned on that one ;-). I'll remove the variable really quick

@trevnorris
Copy link

One question. Otherwise LGTM

per nodejs#14597

Indicate that `'readable'` indicates only that data can
be read from the stream, not that there is actually data
to be consumed. `readable.read([size])` can still return
null. Includes an example that illustrates the point.
@jasnell jasnell force-pushed the doc-improvements3 branch from 1b9d4f3 to 970e2f1 Compare July 6, 2015 18:50
@@ -167,6 +167,32 @@ again when more data is available.
The `readable` event is not emitted in the "flowing" mode with the
sole exception of the last one, on end-of-stream.

Note that the `'readable'` event indicates only that data *can* be
read from the stream. It does not indicate whether there is actual
data to be consumed. The callback passed to handle the `'readable'`

Choose a reason for hiding this comment

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

The first two sentences are pretty confusing. Maybe:

The 'readable' event indicates that the stream has new information: either new data is available or the end of the stream has been reached. In the former case, .read() will return that data. In the latter case, .read() will return null. For instance, in the following example, ...

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will make that change

Copy link
Member Author

Choose a reason for hiding this comment

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

@chrisdickinson ... done... if it looks good to you I'll get it landed

Per nodejs#25635 (comment)

Additional refinement to the clarification on the `readable` event
jasnell pushed a commit that referenced this pull request Jul 10, 2015
Clarifies that emitter.listener() returns a copy, not a reference
Resolves issue #9022

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #25635
jasnell added a commit that referenced this pull request Jul 10, 2015
per: #14596

1. document that a runtime error will occur if you attempt
   to unshift after the end event
2. document that calling read after the end event will return
   null and will not trigger a runtime error

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #25635
jasnell pushed a commit that referenced this pull request Jul 10, 2015
Minor clarifications around Readable._read and Readable.push
to make their implementation/usage easier to understand.

#14124 (comment)

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #25635
jasnell pushed a commit that referenced this pull request Jul 10, 2015
Fix for #25559 (Typo in example of util.deprecate() documentation)

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #25635
jasnell added a commit that referenced this pull request Jul 10, 2015
Per #14604,

Document that performing an `unshift` during a read
can have unexpected results. Following the `unshift`
with a `push('')` resets the reading state appropriately.
Also indicate that doing an `unshift` during a read
is not optimal and should be avoided.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #25635
jasnell added a commit that referenced this pull request Jul 10, 2015
per #14597

Indicate that `'readable'` indicates only that data can
be read from the stream, not that there is actually data
to be consumed. `readable.read([size])` can still return
null. Includes an example that illustrates the point.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #25635
jasnell added a commit that referenced this pull request Jul 10, 2015
Per #25635 (comment)

Additional refinement to the clarification on the `readable` event

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #25635
@jasnell
Copy link
Member Author

jasnell commented Jul 10, 2015

Landed in f91fa52, f7af915, 9faae7a, 423c433, daed421, 340e9f0 and 6036e4f

@jasnell jasnell closed this Jul 10, 2015
jasnell pushed a commit to jasnell/node-joyent that referenced this pull request Jul 10, 2015
Clarifies that emitter.listener() returns a copy, not a reference
Resolves issue nodejs#9022

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25635
jasnell added a commit to jasnell/node-joyent that referenced this pull request Jul 10, 2015
per: nodejs#14596

1. document that a runtime error will occur if you attempt
   to unshift after the end event
2. document that calling read after the end event will return
   null and will not trigger a runtime error

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25635
jasnell pushed a commit to jasnell/node-joyent that referenced this pull request Jul 10, 2015
Minor clarifications around Readable._read and Readable.push
to make their implementation/usage easier to understand.

nodejs#14124 (comment)

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25635
jasnell pushed a commit to jasnell/node-joyent that referenced this pull request Jul 10, 2015
Fix for nodejs#25559 (Typo in example of util.deprecate() documentation)

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25635
jasnell added a commit to jasnell/node-joyent that referenced this pull request Jul 10, 2015
Per nodejs#14604,

Document that performing an `unshift` during a read
can have unexpected results. Following the `unshift`
with a `push('')` resets the reading state appropriately.
Also indicate that doing an `unshift` during a read
is not optimal and should be avoided.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25635
jasnell added a commit to jasnell/node-joyent that referenced this pull request Jul 10, 2015
per nodejs#14597

Indicate that `'readable'` indicates only that data can
be read from the stream, not that there is actually data
to be consumed. `readable.read([size])` can still return
null. Includes an example that illustrates the point.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25635
jasnell added a commit to jasnell/node-joyent that referenced this pull request Jul 10, 2015
Per nodejs#25635 (comment)

Additional refinement to the clarification on the `readable` event

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25635
jasnell added a commit that referenced this pull request Aug 4, 2015
Per #25635 (comment)

Additional refinement to the clarification on the `readable` event

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #25591
jasnell added a commit to jasnell/node that referenced this pull request Aug 4, 2015
Per nodejs/node-v0.x-archive#25635 (comment)

Additional refinement to the clarification on the `readable` event

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs/node-v0.x-archive#25591
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
Per nodejs#25635 (comment)

Additional refinement to the clarification on the `readable` event

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#25591
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants