Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: update comment for needMoreData #19695

Closed
mcollina opened this issue Mar 30, 2018 · 6 comments
Closed

stream: update comment for needMoreData #19695

mcollina opened this issue Mar 30, 2018 · 6 comments
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem.

Comments

@mcollina
Copy link
Member

  • Version: master
  • Platform: all
  • Subsystem: streams

// if it's past the high water mark, we can push in some more.
// Also, if we have no data yet, we can stand some
// more bytes. This is to work around cases where hwm=0,
// such as the repl. Also, if the push() triggered a
// readable event, and the user called read(largeNumber) such that
// needReadable was set, then we ought to push more, so that another
// 'readable' event will be triggered.
function needMoreData(state) {
return !state.ended &&
(state.length < state.highWaterMark ||
state.length === 0);
}
comment is out of date (ref. #19613 (comment)).

Specifically it states:

I think that comment is actually wrong. It says

if it's past the high water mark, we can push in some more.

However it has a check (simplified) as:

return state.length < state.highWaterMark

Which will return true if we are below the watermark.

This would be a good issue for someone willing to learn more about streams.

@mcollina mcollina added good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem. labels Mar 30, 2018
@Xavier-J-Ortiz
Copy link
Contributor

@mcollina I'd like to take this issue up. I do think I'd need some guidance and might have some followup questions while I wrap my head around the problem.

@mcollina
Copy link
Member Author

Definitely. If that also result in one more unit test, even better!
Ping me if you have any questions!

@vsnehil92
Copy link
Contributor

@mcollina, since there is no PR from 6 days, may I work on this issue? This is my first contribution so I would need some guidance.

@mcollina
Copy link
Member Author

mcollina commented Apr 5, 2018

Sure, feel free to ping me in IRC or twitter any time.

@Xavier-J-Ortiz
Copy link
Contributor

Xavier-J-Ortiz commented Apr 5, 2018

@mcollina - Apologies, something came up on my end.

@vsnehil92 - Feel free to work on this as I haven't had any time.

@lpinca
Copy link
Member

lpinca commented Apr 15, 2018

Fixed by e76831b. Closing.

@lpinca lpinca closed this as completed Apr 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants