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.

Sign up for GitHub

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

Jump to bottom

streams: readable.setEncoding vs writable.setDefaultEncoding #5013

Closed
thurt opened this issue Jan 31, 2016 · 3 comments
Closed

streams: readable.setEncoding vs writable.setDefaultEncoding #5013

thurt opened this issue Jan 31, 2016 · 3 comments
Labels
question Issues that look for answers. stream Issues and PRs related to the stream subsystem.

Comments

@thurt
Copy link

thurt commented Jan 31, 2016

Is there a reason why these two do not match each other in method name, and shouldn't they both return this?
@thurt thurt changed the title readable.setEncoding vs writable.setDefaultEncoding streams: readable.setEncoding vs writable.setDefaultEncoding Jan 31, 2016
@mscdex mscdex added question Issues that look for answers. stream Issues and PRs related to the stream subsystem. labels Jan 31, 2016
@evanlucas
Copy link
Contributor

The writable stream uses setDefaultEncoding because one can pass the encoding on each write() call. If encoding is not passed to write, then the default encoding is used. The readable stream uses whatever the current encoding is on every chunk that comes in.

Feel free to open a PR for Writable#setDefaultEncoding returning this.

Thanks!

@estliberitas estliberitas mentioned this issue Feb 2, 2016
Merged
@cjihrig
Copy link
Contributor

cjihrig commented Feb 2, 2016

This would have probably made for a good good first contribution label.

@evanlucas
Copy link
Contributor

ah, good point. My bad

calvinmetcalf pushed a commit that referenced this issue Apr 30, 2016
@estliberitas @calvinmetcalf
doc: make writable.setDefaultEncoding() return this
bcce05d 
Let this function return `this` for parity with `readable.setEncoding()`.

PR-URL: #5040
Fixes: #5013

Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this issue May 4, 2016
@estliberitas @Fishrock123
doc: make writable.setDefaultEncoding() return this
e6c8da4 
Let this function return `this` for parity with `readable.setEncoding()`.

PR-URL: #5040
Fixes: #5013

Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue May 4, 2016
@estliberitas @joelostrowski
doc: make writable.setDefaultEncoding() return this
f420583 
Let this function return `this` for parity with `readable.setEncoding()`.

PR-URL: nodejs#5040
Fixes: nodejs#5013

Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@cypress cypress bot mentioned this issue Nov 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
question Issues that look for answers. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@mscdex @evanlucas @cjihrig @thurt