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

changed comment that was wrong in _steram_readable.js #19882

Closed
wants to merge 7 commits into from

Conversation

vsnehil92
Copy link
Contributor

This is the PR with respect to the issue #19695 @mcollina please review the comment.

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Apr 8, 2018
@@ -302,7 +302,8 @@ function chunkInvalid(state, chunk) {
}


// if it's past the high water mark, we can push in some more.
// It simply checks the the state length with high water mark
Copy link
Member

Choose a reason for hiding this comment

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

Typo: the the

Copy link
Member

Choose a reason for hiding this comment

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

The previous sentence stated that we could push more if we were past the highWaterMark.
I would like to have a similar sentence there.
This function returns the result of .push().

@@ -302,7 +302,8 @@ function chunkInvalid(state, chunk) {
}


// if it's past the high water mark, we can push in some more.
// It simply checks the the state length with high water mark
// which will return true if we are below the watermark
// Also, if we have no data yet, we can stand some
Copy link
Member

Choose a reason for hiding this comment

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

Missing period?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no it isn't missing period made it more clear in the latest commit

// if it's past the high water mark, we can push in some more.
// It simply checks the state length with high water mark
// and will return true if we are below the watermark.
// We can also push more code if were past the highWaterMark.
Copy link
Member

Choose a reason for hiding this comment

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

We can also push more code if were past the highWaterMark.

Are you sure about this? If we are past the highWaterMark, this will return false, according to our docs this should signal "please stop".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes we can't push more update the complete comment and making is similar to the previous one

Copy link
Member

Choose a reason for hiding this comment

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

Ok, nit: "were" -> "we are".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok thanks, done the changes

@@ -302,7 +302,7 @@ function chunkInvalid(state, chunk) {
}


// if it's past the high water mark, we can push in some more.
// We can push more code if are below the highWaterMark.
Copy link
Member

Choose a reason for hiding this comment

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

if we are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry made changes now everything looks good please review

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -302,7 +302,7 @@ function chunkInvalid(state, chunk) {
}


// if it's past the high water mark, we can push in some more.
// We can push more code if we are below the highWaterMark.
Copy link
Member

Choose a reason for hiding this comment

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

s/code/data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may you please elaborate

Copy link
Member

Choose a reason for hiding this comment

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

pushing "more code" is not very clear.

@BridgeAR
Copy link
Member

@vsnehil92 would you be so kind and address the comment? Otherwise this should be good to go.

@vsnehil92
Copy link
Contributor Author

Updating more code to more data

@vsnehil92
Copy link
Contributor Author

updated the comment sorry for the delay

@jasnell
Copy link
Member

jasnell commented Apr 14, 2018

jasnell pushed a commit that referenced this pull request Apr 14, 2018
PR-URL: #19882
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 14, 2018

Landed in e76831b

@jasnell jasnell closed this Apr 14, 2018
jasnell pushed a commit that referenced this pull request Apr 16, 2018
PR-URL: #19882
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
PR-URL: nodejs#19882
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants