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: minor fix for ReadableState #11139

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@cynron
Contributor

cynron commented Feb 3, 2017

  1. remove unused flag ranOut from ReadableState, ranOut had been unused since 0f8de5e

  2. fix the comments for flag sync of ReadableState.

Checklist
  • make -j4 test (UNIX) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • stream

Wang Xinyong added some commits Feb 3, 2017

Wang Xinyong
stream: remove unused `ranOut` from ReadableState
flag `ranOut` became unused since 0f8de5e,
but 0f8de5e did not remove it.
Wang Xinyong
stream: fix comments for sync flag of ReadableState
It seems that the comments for sync flag of ReadableState is
copied from WritableState without modification.

@cynron cynron changed the title from Minor fix for ReadableState to stream: minor fix for ReadableState Feb 3, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
Member

jasnell commented Feb 3, 2017

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 3, 2017

Member

Given that this removes a property, this is potentially semver-major. It shouldn't be, but we need to be certain.

Member

jasnell commented Feb 3, 2017

Given that this removes a property, this is potentially semver-major. It shouldn't be, but we need to be certain.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 3, 2017

Member

I think we are going to release this as semver-minor in readable-stream. If that does not cause issues, we can release this as semver-minor also in Node.

ReadableState  is undocumented atm.

Member

mcollina commented Mar 3, 2017

I think we are going to release this as semver-minor in readable-stream. If that does not cause issues, we can release this as semver-minor also in Node.

ReadableState  is undocumented atm.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 3, 2017

Member

Landed as 1004b9b and 202b07f

Member

mcollina commented Mar 3, 2017

Landed as 1004b9b and 202b07f

@mcollina mcollina closed this Mar 3, 2017

mcollina added a commit that referenced this pull request Mar 3, 2017

stream: remove unused `ranOut` from ReadableState
flag `ranOut` became unused since 0f8de5e,
but 0f8de5e did not remove it.

PR-URL: #11139
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

mcollina added a commit that referenced this pull request Mar 3, 2017

stream: fix comment for sync flag of ReadableState
It seems that the comment for sync flag of ReadableState is
copied from WritableState without modification.

PR-URL: #11139
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Mar 4, 2017

Member

@mcollina Just ICYMI, our current policy is that at least 2 CTC members need to sign off on semver-major changes before landing them. I don’t think that’s a problem here because it’s not something that’s really covered by our API (as you pointed out)… I just added the label because, at least from Node’s perspective, I don’t think there’s a reason to not consider this semver-major.

Member

addaleax commented Mar 4, 2017

@mcollina Just ICYMI, our current policy is that at least 2 CTC members need to sign off on semver-major changes before landing them. I don’t think that’s a problem here because it’s not something that’s really covered by our API (as you pointed out)… I just added the label because, at least from Node’s perspective, I don’t think there’s a reason to not consider this semver-major.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Mar 4, 2017

Member

Oh, I thought this was sitting there for a while, and just reduced the pull requests count by one.
Thanks for noting.

Member

mcollina commented Mar 4, 2017

Oh, I thought this was sitting there for a while, and just reduced the pull requests count by one.
Thanks for noting.

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment