Minor clarifications around Readable._read and Readable.push #25581
Conversation
| The `Readable` class works by putting data into a read queue to be | ||
| pulled out later by calling the `read()` method when the `'readable'` | ||
| event fires. | ||
| If a String or Buffer is passed, The `push()` method adds a chunk of data |
There was a problem hiding this comment.
This gets a little into the weeds, but this should be "if a non-null value is passed, the push() method adds it into the queue for subsequent..." (taking into account objectMode.)
|
Changed the wording as per chris dickinson's comment |
|
Looks like a good change overall. Would appreciate if you could squash the commits down into a single commit and add a bit more of a summary to the commit message (helpful but not critical). Thank you! |
|
@jasnell Is there an easy way to do that in github or do I have to pull the whole repo? |
|
Ugh, what an unintuitive process. Ok I squashed the commits into this one: fresheneesz@b2d15f3 . What do I do now? Close this one and create a new pull request? |
…their implementation/usage easier to understand. nodejs#14124 (comment)
|
Nope! Nothing further is necessary on your part. I'll take another pass at reviewing this shortly then will work on getting it landed (likely next week). Thank you! |
|
Ok! Sure thing! |
|
LGTM. @chrisdickinson @trevnorris |
|
LGTM |
|
@fresheneesz ... unfortunately, this appears to have been clobbered a bit by another change that was landed. If you'd like to rebase and resolve the conflict, that would be great. I'll get it landed right after. Otherwise, once I'm through a few other items I'll see if I can get to rebasing this. |
|
: / , I don't have time right now to figure that out, but maybe in a couple weeks. |
|
No worries, if I get the time before then I'll untangle it and point here with the new commit. |
|
Ok landed this in 4952e2b and over in io.js as well. |
#14124 (comment)