-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: remove ambiguous code #29664
Conversation
s/ambigious/ambiguous/ in commit message |
2971a38
to
67be60d
Compare
Not sure what you meant. But I'm guess it's the misspelling. Fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @mafintosh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR-URL: #29664 Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Landed in ce62e96 |
@ronag I guess this depends on other PRs that need backporting first? This currently causes conflicts that are not obvious to resolve. Would you be so kind and check if other PRs need backporting first and add that as well / open a backport if you think that's useful here? |
This is probably about to be reverted in #29717 (comment). I will look into backporting this if/when this becomes relevant again. |
quick ping re: backport |
PR-URL: nodejs#29664 Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#29664 Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins In view of 8c60685#diff-009356850b536cab27b019ba8ad15e72 are you sure this should be backported? |
Seems like it shouldn't |
This cleans up some ambiguous code. I don't believe this actually causes any observable change of behaviour.
writable
,readable
is supposed to indicate the type of the stream, which does not change... this PR makes the code less confusingChecklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes