Skip to content

stream,doc: make writable.setDefaultEncoding() return this#5040

Merged
calvinmetcalf merged 1 commit into
nodejs:masterfrom
estliberitas:stream-return-this
Apr 30, 2016
Merged

stream,doc: make writable.setDefaultEncoding() return this#5040
calvinmetcalf merged 1 commit into
nodejs:masterfrom
estliberitas:stream-return-this

Conversation

@estliberitas
Copy link
Copy Markdown
Contributor

@estliberitas estliberitas commented Feb 2, 2016

Let this function return this for parity with readable.setEncoding() though they set encodings for different purposes. (#5013).
@cjihrig
Copy link
Copy Markdown
Contributor

cjihrig commented Feb 2, 2016

LGTM
@cjihrig cjihrig added stream Issues and PRs related to the stream subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 2, 2016
@evanlucas
Copy link
Copy Markdown
Contributor

evanlucas commented Feb 2, 2016

LGTM
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Feb 2, 2016

LGTM
CI: https://ci.nodejs.org/job/node-test-pull-request/1510/
@mscdex
Copy link
Copy Markdown
Contributor

mscdex commented Feb 2, 2016

Wouldn't this be semver-major?
@cjihrig
Copy link
Copy Markdown
Contributor

cjihrig commented Feb 2, 2016

I guess it's debatable. I gave it semver-minor because it's adding a return type where one didn't exist before, but I can definitely see the argument for semver-major. I don't have a strong opinion.
@evanlucas
Copy link
Copy Markdown
Contributor

evanlucas commented Feb 3, 2016

I know we landed some changes like this previously for net (#1768). They were landed as semver-minor, but I don't really have a strong opinion here either.
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Feb 3, 2016

Since it never returned a value previously there shouldn't be anyone depending on that return value ;-) ... it's likely safe as a semver-minor.
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 2, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/2135/
@estliberitas estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:23
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 28, 2016

New New CI since this didn't land before: https://ci.nodejs.org/job/node-test-pull-request/2419/
@estliberitas
Copy link
Copy Markdown
Contributor Author

estliberitas commented Apr 29, 2016

I guess, first I have to resolve conflicts
@estliberitas
stream,doc: make writable.setDefaultEncoding() return this
46f4f9a 
Let this function return `this` for parity with `readable.setEncoding()` (#5013).
@estliberitas
Copy link
Copy Markdown
Contributor Author

estliberitas commented Apr 29, 2016

@jasnell OS X build failed on test-tick-processor and I guess it's not relevant?
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 30, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/2442/
Yes, the test-tick-processor was an unrelated issue.
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 30, 2016

CI is green.
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Apr 30, 2016

@nodejs/streams ... any objections to this one?
@mcollina
Copy link
Copy Markdown
Member

mcollina commented Apr 30, 2016

LGTM
@sonewman
Copy link
Copy Markdown
Contributor

sonewman commented Apr 30, 2016

LGTM 👍
@calvinmetcalf calvinmetcalf merged commit bcce05d into nodejs:master Apr 30, 2016
Fishrock123 pushed a commit that referenced this pull request 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>
This was referenced May 4, 2016
Closed
Merged
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request 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>
Fishrock123 added a commit that referenced this pull request May 5, 2016
@Fishrock123
2016-05-05, Version 6.1.0 (Current)
cd7b76e 
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
  - Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
  - Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
    - This is set to `100` by default.
  - Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
    - Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557
Fishrock123 added a commit that referenced this pull request May 5, 2016
@Fishrock123
2016-05-05, Version 6.1.0 (Current)
7f82130 
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
  - Please see our blog post for more info on the security contents of this release:
  - https://nodejs.org/en/blog/vulnerability/openssl-may-2016/
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
  - Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
  - Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
    - This is set to `100` by default.
  - Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
    - Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557
Fishrock123 added a commit that referenced this pull request May 5, 2016
@Fishrock123
2016-05-05, Version 6.1.0 (Current)
b9ceb42 
* assert: `deep{Strict}Equal()` now works correctly with circular
references. (Rich Trott) #6432
* debugger: Arrays are now formatted correctly in the debugger repl.
(cjihrig) #6448
* deps: Upgrade OpenSSL sources to 1.0.2h (Shigeki Ohtsu)
#6550
- Please see our blog post for more info on the security contents of
this release:
- https://nodejs.org/en/blog/vulnerability/openssl-may-2016/
* net: Introduced a `Socket#connecting` property. (Fedor Indutny)
#6404
- Previously this information was only available as the undocumented,
internal `_connecting` property.
* process: Introduced `process.cpuUsage()`. (Patrick Mueller)
#6157
* stream: `Writable#setDefaultEncoding()` now returns `this`.
(Alexander Makarenko) #5040
* util: Two new additions to `util.inspect()`:
- Added a `maxArrayLength` option to truncate the formatting of
Arrays. (James M Snell) #6334
- This is set to `100` by default.
- Added a `showProxy` option for formatting proxy intercepting
handlers. (James M Snell) #6465
- Inspecting proxies is non-trivial and as such this is off by
default.

PR-URL: #6557
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-minor PRs that contain new features and should be released in the next minor version. stream Issues and PRs related to the stream subsystem.

8 participants

@estliberitas @cjihrig @evanlucas @jasnell @mscdex @mcollina @sonewman @calvinmetcalf