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

Update stream.md - fix issue in setEncoding method #11363

Closed
wants to merge 1 commit into from
Closed

Update stream.md - fix issue in setEncoding method #11363

wants to merge 1 commit into from

Conversation

RickBullotta
Copy link
Contributor

Removed an incorrect reference to the use of setEncoding(null) as the proper way to handling binary streams or to disable encoding, and explained that the default encoding is "no encoding", and that this is the correct approach for dealing with binary data via Buffers.

Checklist
  • documentation is changed or added
Affected core subsystem(s)

Affects documentation only

Removed an incorrect reference to the use of setEncoding(null) as the proper way to handling binary streams or to disable encoding, and explained that the default encoding is "no encoding", and that this is the correct approach for dealing with binary data via Buffers.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Feb 14, 2017
The `readable.setEncoding()` method sets the default character encoding for
data read from the Readable stream.
The `readable.setEncoding()` method sets the character encoding for
data read from the Readable stream.
Copy link
Contributor

Choose a reason for hiding this comment

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

Trailing space should be removed.


Setting an encoding causes the stream data
By default, no encoding is assigned and stream data will be returned as
Buffer objects. Setting an encoding causes the stream data
Copy link
Contributor

Choose a reason for hiding this comment

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

Double space after the period should be a single space (IMHO and to match the surrounding text).


Setting an encoding causes the stream data
By default, no encoding is assigned and stream data will be returned as
Buffer objects. Setting an encoding causes the stream data
to be returned as string of the specified encoding rather than as `Buffer`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add an 'a' before 'string' here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

and before `Buffer`

Copy link
Member

Choose a reason for hiding this comment

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

Can you add an 'a' before 'string' here too?

Went with strings and Buffers instead (to match the rest of the paragraph).

@mscdex
Copy link
Contributor

mscdex commented Feb 14, 2017

The first line of the commit message should target the doc subsystem.

@gibfahn
Copy link
Member

gibfahn commented Feb 14, 2017

Probably worth adding Fixes: #11352 and Refs: #11316 lines to the commit message. With @mscdex's nit above, you could probably do this for the commit message:

 doc: don't suggest setEncoding for binary streams

Removed an incorrect reference to the use of setEncoding(null) as the proper
way to handling binary streams or to disable encoding, and explained that the
default encoding is "no encoding", and that this is the correct approach for
dealing with binary data via Buffers.

Fixes: https://github.com/nodejs/node/issues/11352
Refs: https://github.com/nodejs/node/issues/11316

cc/ @sam-github for review


Setting an encoding causes the stream data
By default, no encoding is assigned and stream data will be returned as
Buffer objects. Setting an encoding causes the stream data
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you format Buffer as inline code.

Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

LGTM, once @mscdex and @cjihrig 's comments have been addressed

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 when the others nits are addressed.

@mcollina
Copy link
Member

@RickBullotta would you mind addressing the relative nits?
Otherwise, may I land and address the nit while landing this?

@RickBullotta
Copy link
Contributor Author

RickBullotta commented May 19, 2017 via email

@gibfahn gibfahn self-assigned this May 19, 2017
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Will fix nits on landing.

@gibfahn gibfahn dismissed cjihrig’s stale review May 19, 2017 12:02

Will fix on landing.

@gibfahn
Copy link
Member

gibfahn commented May 19, 2017

Landed in 8476880

@gibfahn gibfahn closed this May 19, 2017
gibfahn pushed a commit that referenced this pull request May 19, 2017
Removed an incorrect reference to the use of setEncoding(null) as the
proper way to handling binary streams or to disable encoding, and
explained that the default encoding is "no encoding", and that this is
the correct approach for dealing with binary data via Buffers.

PR-URL: #11363
Fixes: #11352
Refs: #11316
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@gibfahn
Copy link
Member

gibfahn commented May 19, 2017

Have at it. The amount of nit picking turned me off to the whole process.

@RickBullotta I'd be really interested to get more feedback from you on this. What would your ideal process be?

Questions:
  • If we had a doc linter that could catch the majority of these nits would you be more willing to fix them?
  • Do you think we're being too picky in general, and should just not worry about formatting?
  • Is the problem in the way the nits were raised?
  • Do you think collaborators should just fix everything on landing and not expect contributors to fix nits in PRs?

There's no right answer for these things, but we're always trying to improve our process, so feedback from people just starting to get involved with the project is very helpful.

anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
Removed an incorrect reference to the use of setEncoding(null) as the
proper way to handling binary streams or to disable encoding, and
explained that the default encoding is "no encoding", and that this is
the correct approach for dealing with binary data via Buffers.

PR-URL: nodejs#11363
Fixes: nodejs#11352
Refs: nodejs#11316
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@RickBullotta
Copy link
Contributor Author

RickBullotta commented May 19, 2017 via email

@MylesBorins
Copy link
Member

is this applicable for v6.x?

@mcollina
Copy link
Member

@MylesBorins yes you can backport.

MylesBorins pushed a commit that referenced this pull request Jul 17, 2017
Removed an incorrect reference to the use of setEncoding(null) as the
proper way to handling binary streams or to disable encoding, and
explained that the default encoding is "no encoding", and that this is
the correct approach for dealing with binary data via Buffers.

PR-URL: #11363
Fixes: #11352
Refs: #11316
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants