Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

streams: set default encoding for writable streams #8483

Closed
wants to merge 10 commits into from

Conversation

recursivefunk
Copy link

This commit adds the ability to set the default encoding for writable streams.

Fixes #7159

@misterdjules
Copy link

@jrayaustin Thank you! I haven't checked the code, but you'll need to make your commit conform to the commit messages guidelines. More specifically "The first line must be no more than 50 characters".

This commit adds the ability to set the default
encoding for writable streams.

Fixes nodejs#7159
@recursivefunk recursivefunk changed the title streams: adds the ability to set the default encoding for writable strea... streams: set default encoding for writable streams Oct 2, 2014
@recursivefunk
Copy link
Author

Ah, got it. Just made the change thanks for your guidance on this @misterdjules

yorkie and others added 5 commits October 3, 2014 01:31
Add generic functions for (U)Int read/write operations on Buffers. These
support up to and including 48 bit reads and writes.

Include documentation and tests.

Additional work done by Trevor Norris to include 40 and 48 bit write
support. Because bitwise operations cannot be used on values greater
than 32 bits, the operations have been replaced with mathematical
calculations. Regardless, they are still faster than floating point
operations.

Reviewed-by: Trevor Norris <trev.norris@gmail.com>
* `util.inspect` cannot accept es6 symbol primitive
* It will throw exception if do `util.inspect(Symbol())`
* This also affects repl, console.log, etc.

Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-by: Chris Dickinson <christopher.s.dickinson@gmail.com>
Associates link to dns.lookup() with proper URL.

Fixes: nodejs#8018
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-by: Chris Dickinson <christopher.s.dickinson@gmail.com>
The docs for readLine.pause are misleading. I seriously spent hours on this. If
it isn't a bug, at least it should be well documented.

Someone else stumbled on this too:
http://stackoverflow.com/questions/21341050/pausing-readline-in-node-js

Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-by: Chris Dickinson <christopher.s.dickinson@gmail.com>
Fixes: nodejs#8458
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-by: Chris Dickinson <christopher.s.dickinson@gmail.com>
Writable.prototype.setDefaultEncoding = function(encoding) {
if (encoding) {
this._writableState.defaultEncoding = encoding;
}

Choose a reason for hiding this comment

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

Few things. We're being better about naming the functions. Make sure the encoding is a string. Also, no need for the braces in a single line if.

johnny-ray added 4 commits October 7, 2014 18:18
This commit adds the ability to set the default
encoding for writable streams.

Fixes nodejs#7159
@recursivefunk
Copy link
Author

@trevnorris take another look. In addition to checking for a string type i added a return value of true if the encoding was successfully changed otherwise a false value is returned. Seem appropriate?

trevnorris pushed a commit that referenced this pull request Oct 8, 2014
Add API Writable#setDefaultEncoding().

PR-URL: #8483
Fixes: #7159
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
@trevnorris
Copy link

Thanks much for the contribution! Squashed and merged in f04f3a0. Made a couple changes:

  • Ensure encoding is lower case. Required by node::ParseEncoding().
  • Check encoding against Buffer.isEncoding().
  • Added one additional test to verify encoding is turned to lower case.

@recursivefunk
Copy link
Author

Sweeet!

@mscdex
Copy link

mscdex commented Oct 8, 2014

The "return boolean" behavior seems inconsistent with the likes of readable.setEncoding('badencodingname') which throws on bad encoding names. I'd expect writable.setDefaultEncoding('badencodingname') to throw also...

@recursivefunk
Copy link
Author

Interesting point @mscdex if @trevnorris thinks it's appropriate I'll gladly make the change.

@mscdex
Copy link

mscdex commented Oct 9, 2014

@jrayaustin I submitted #8529 yesterday.

@recursivefunk
Copy link
Author

Oh nice, thanks @mscdex

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants