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

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

Merged
merged 1 commit into from Apr 30, 2016

Conversation

Projects
None yet
8 participants
@estliberitas
Member

estliberitas commented Feb 2, 2016

Let this function return this for parity with readable.setEncoding() though they set encodings for different purposes. (#5013).

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Feb 2, 2016

Contributor

LGTM

Contributor

cjihrig commented Feb 2, 2016

LGTM

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Feb 2, 2016

Member

LGTM

Member

evanlucas commented Feb 2, 2016

LGTM

@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Feb 2, 2016

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 2, 2016

Contributor

Wouldn't this be semver-major?

Contributor

mscdex commented Feb 2, 2016

Wouldn't this be semver-major?

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Feb 2, 2016

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Feb 3, 2016

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 3, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Apr 2, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 28, 2016

Member

New New CI since this didn't land before: https://ci.nodejs.org/job/node-test-pull-request/2419/

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/

@jasnell jasnell removed the lts-watch-v4.x label Apr 28, 2016

@estliberitas

This comment has been minimized.

Show comment
Hide comment
@estliberitas

estliberitas Apr 29, 2016

Member

I guess, first I have to resolve conflicts

Member

estliberitas commented Apr 29, 2016

I guess, first I have to resolve conflicts

stream,doc: make `writable.setDefaultEncoding()` return `this`
Let this function return `this` for parity with `readable.setEncoding()` (#5013).
@estliberitas

This comment has been minimized.

Show comment
Hide comment
@estliberitas

estliberitas Apr 29, 2016

Member

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

Member

estliberitas commented Apr 29, 2016

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 30, 2016

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 30, 2016

Member

CI is green.

Member

jasnell commented Apr 30, 2016

CI is green.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 30, 2016

Member

@nodejs/streams ... any objections to this one?

Member

jasnell commented Apr 30, 2016

@nodejs/streams ... any objections to this one?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Apr 30, 2016

Member

LGTM

Member

mcollina commented Apr 30, 2016

LGTM

@sonewman

This comment has been minimized.

Show comment
Hide comment
@sonewman

sonewman Apr 30, 2016

Contributor

LGTM 👍

Contributor

sonewman commented Apr 30, 2016

LGTM 👍

@calvinmetcalf calvinmetcalf merged commit bcce05d into nodejs:master Apr 30, 2016

Fishrock123 added a commit that referenced this pull request May 4, 2016

doc: make `writable.setDefaultEncoding()` return `this`
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 added a commit to joelostrowski/node that referenced this pull request May 4, 2016

doc: make `writable.setDefaultEncoding()` return `this`
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

2016-05-05, Version 6.1.0 (Current)
* 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

2016-05-05, Version 6.1.0 (Current)
* 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

2016-05-05, Version 6.1.0 (Current)
* 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