doc: document switchover from measured bytes to chars after setEncoding by jalafel · Pull Request #13442 · nodejs/node

jalafel · 2017-06-03T20:15:46Z

Checklist

This is documentation PR to denote the change of measurement against the highWaterMark in non-object streams after .setEncoding() is called. It changes from bytes to character.

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

vsemozhetbyt · 2017-06-03T20:25:28Z

doc/api/stream.md

It seems this line exceeds 80 characters limit.

vsemozhetbyt · 2017-06-03T20:26:49Z

doc/api/stream.md

Maybe these empty lines are unintentional?

mscdex · 2017-06-03T21:37:48Z

doc/api/stream.md

I think we are unofficially targeting US English, so s/behaviour/behavior.

mscdex · 2017-06-03T21:37:58Z

doc/api/stream.md

Ditto here.

mscdex · 2017-06-03T21:41:47Z

doc/api/stream.md

I think 'atypically' here should be 'typically'?

mscdex · 2017-06-03T21:44:06Z

Also, the first line of the commit message is too long.

tniessen · 2017-08-16T10:55:55Z

@jessicaquynh This needs to be rebased, sorry for the delay.
@mscdex @vsemozhetbyt Could you PTAL?

BridgeAR · 2017-08-28T00:01:11Z

Ping @jessicaquynh this needs a rebase

jalafel · 2017-08-28T04:26:30Z

@BridgeAR Sorry! Didn't realize that was on me. Thank you!

mscdex · 2017-08-28T04:32:30Z

doc/api/stream.md

I think the casing for the parameter type should stay the same.

mscdex · 2017-08-28T04:32:36Z

doc/api/stream.md

mscdex · 2017-08-28T04:33:48Z

doc/api/stream.md

Perhaps italicize the word 'characters' here as well since it is done for the word 'bytes' in the previous sentence.

mscdex · 2017-08-28T04:36:30Z

doc/api/stream.md

This sentence seems a little confusing to me since we're comparing a multi-byte encoding and a single-byte encoding here. The way the sentence reads to me is that 'ascii' and 'utf8' are common cases and I do not need to worry about them, which is not true for multi-byte utf8 characters. If we're going to name some example encodings, you might replace 'utf8' with 'latin1' instead.

mscdex · 2017-08-28T04:37:28Z

doc/api/stream.md

I think it might be better to be more specific about what 'special' means, which is 'multi-byte'.

Also, the length of the string has no bearing on this particular problem. You could have a single character that spans 4 bytes for utf8 for example.

BridgeAR

LGTM with the comment addressed.

BridgeAR · 2017-08-30T19:08:13Z

doc/api/stream.md

The last sentence does not seem to be complete. I guess there is a with missing after "working". I would also add a could after "that" as not every string would likely be an issue.

This commit documents and edge-case behavior in readable streams. It is expected that non-object streams are measured in bytes against the highWaterMark. However, it was discovered in issue nodejs#6798 that after calling .setEncoding() on the stream, it will thereafter begin to measure the buffer's length in characters.

BridgeAR · 2017-09-08T02:23:35Z

Landed in 89f2074

@jessicaquynh thanks a lot for your contribution

This commit documents and edge-case behavior in readable streams. It is expected that non-object streams are measured in bytes against the highWaterMark. However, it was discovered in issue thereafter begin to measure the buffer's length in characters. PR-URL: #13442 Refs: #6798 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This commit documents and edge-case behavior in readable streams. It is expected that non-object streams are measured in bytes against the highWaterMark. However, it was discovered in issue thereafter begin to measure the buffer's length in characters. PR-URL: nodejs#13442 Refs: nodejs#6798 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

This commit documents and edge-case behavior in readable streams. It is expected that non-object streams are measured in bytes against the highWaterMark. However, it was discovered in issue thereafter begin to measure the buffer's length in characters. PR-URL: #13442 Refs: #6798 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jun 3, 2017

vsemozhetbyt reviewed Jun 3, 2017

View reviewed changes

doc/api/stream.md Outdated

Copy link

Contributor

vsemozhetbyt Jun 3, 2017

Choose a reason for hiding this comment

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

It seems this line exceeds 80 characters limit.

vsemozhetbyt reviewed Jun 3, 2017

View reviewed changes

mscdex reviewed Jun 3, 2017

View reviewed changes

doc/api/stream.md Outdated

Copy link

Contributor

mscdex Jun 3, 2017

Choose a reason for hiding this comment

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

I think we are unofficially targeting US English, so s/behaviour/behavior.

mscdex reviewed Jun 3, 2017

View reviewed changes

doc/api/stream.md Outdated

Copy link

Contributor

mscdex Jun 3, 2017

Choose a reason for hiding this comment

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

Ditto here.

mscdex reviewed Jun 3, 2017

View reviewed changes

doc/api/stream.md Outdated

Copy link

Contributor

mscdex Jun 3, 2017

Choose a reason for hiding this comment

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

I think 'atypically' here should be 'typically'?

jalafel force-pushed the doc-#6798 branch 3 times, most recently from 0231823 to 9205f3f Compare June 4, 2017 19:24

jasnell approved these changes Jun 4, 2017

View reviewed changes

vsemozhetbyt approved these changes Aug 16, 2017

View reviewed changes

jalafel force-pushed the doc-#6798 branch from d43c809 to 111f17a Compare August 28, 2017 04:24

mscdex reviewed Aug 28, 2017

View reviewed changes

doc/api/stream.md Outdated

Copy link

Contributor

mscdex Aug 28, 2017

Choose a reason for hiding this comment

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

I think the casing for the parameter type should stay the same.

mscdex reviewed Aug 28, 2017

View reviewed changes

doc/api/stream.md Outdated

Copy link

Contributor

mscdex Aug 28, 2017

Choose a reason for hiding this comment

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

Ditto.

mscdex reviewed Aug 28, 2017

View reviewed changes

jalafel force-pushed the doc-#6798 branch 2 times, most recently from 52246a0 to d0094d9 Compare August 29, 2017 15:10

BridgeAR approved these changes Aug 30, 2017

View reviewed changes

jalafel force-pushed the doc-#6798 branch from d0094d9 to 6e8ecc2 Compare August 31, 2017 00:31

BridgeAR approved these changes Sep 8, 2017

View reviewed changes

BridgeAR closed this Sep 8, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the land-on-v6.x label Sep 20, 2017

MylesBorins mentioned this pull request Sep 20, 2017

v6.11.4 proposal #15506

Merged

gireeshpunathil mentioned this pull request Apr 9, 2018

streams: Readable highWaterMark is measured in bytes *or* characters #6798

Closed

Uh oh!

Conversation

jalafel commented Jun 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jun 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Aug 16, 2017

Uh oh!

BridgeAR commented Aug 28, 2017

Uh oh!

jalafel commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jalafel commented Jun 3, 2017 •

edited

Loading

mscdex commented Jun 3, 2017 •

edited

Loading