FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: fix pipeline pump by ronag · Pull Request #39006 · nodejs/node · GitHub

/ node Public

stream: fix pipeline pump - #39006

Closed
ronag wants to merge 13 commits into
nodejs:masterfrom
nxtedition:fix-pipeline-pump
ConversationCommits (13)ChecksFiles changed
Closed

stream: fix pipeline pump#39006
ronag wants to merge 13 commits into
nodejs:masterfrom
nxtedition:fix-pipeline-pump

Conversation

ronag commented Jun 11, 2021

Copy link
Copy Markdown
Member

Refs: #39005

ronag added the stream Issues and PRs related to the stream subsystem. label Jun 11, 2021
ronag requested a review from mcollina June 11, 2021 09:16

ronag commented Jun 11, 2021

Copy link
Copy Markdown
Member Author

Needs tests.

github-actions Bot added the needs-ci PRs that need a full CI run. label Jun 11, 2021
ronag force-pushed the fix-pipeline-pump branch from 1b18705 to 963f536 Compare June 11, 2021 09:58

This comment has been minimized.

Sign in to view

This comment has been minimized.

Sign in to view

ronag commented Jun 11, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

@VoltrexMaster Thanks for the help. Why not make github change suggestions? That way I can just commit your changes directly.

In general it's better to review the code in the code instead of in comments.

ronag commented Jun 11, 2021
View reviewed changes
Comment thread lib/internal/streams/pipeline.js

VoltrexKeyva commented Jun 11, 2021

Copy link
Copy Markdown
Contributor

@ronag yea sorry, I already know how those work; just placed those in a comment as github doesn't allow suggestions in unchanged lines which for example the const { line, wanted to be more specific if you know what I mean 😅

RaisinTen reviewed Jun 11, 2021
View reviewed changes
Comment thread lib/internal/streams/pipeline.js Outdated
Update lib/internal/streams/pipeline.js
1cd6a1c 
Co-authored-by: Darshan Sen <raisinten@gmail.com>
aduh95 reviewed Jun 11, 2021
View reviewed changes
Comment thread lib/internal/streams/pipeline.js
Comment thread lib/internal/streams/pipeline.js Outdated
Comment thread lib/internal/streams/pipeline.js Outdated
ronag and others added 2 commits June 11, 2021 15:24
Apply suggestions from code review
33c4e21 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
fixup
30b7e8d
aduh95 reviewed Jun 11, 2021
View reviewed changes
Comment thread lib/internal/streams/pipeline.js Outdated
Comment thread lib/internal/streams/pipeline.js Outdated
mcollina requested changes Jun 11, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you please add a unit test?

ronag and others added 3 commits June 11, 2021 16:27
Apply suggestions from code review
e6a62f4 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
fixup: test
2b453dc
fixup: test
f12cc3d
mcollina requested changes Jun 11, 2021
View reviewed changes
Comment thread lib/internal/streams/pipeline.js
aduh95 reviewed Jun 11, 2021
View reviewed changes
Comment thread lib/internal/streams/pipeline.js Outdated
Update lib/internal/streams/pipeline.js
4e6be16 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
mcollina approved these changes Jun 12, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

ronag added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 12, 2021

nodejs-github-bot commented Jun 12, 2021 •
edited by ronag
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38595/ ✅

ronag commented Jun 12, 2021

Copy link
Copy Markdown
Member Author

@nodejs/streams

mcollina approved these changes Jun 12, 2021
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

jasnell commented Jun 14, 2021

Copy link
Copy Markdown
Member

Landed in bdcb738

jasnell closed this Jun 14, 2021
jasnell pushed a commit that referenced this pull request Jun 14, 2021
stream: fix pipeline pump
bdcb738 
Refs: #39005

PR-URL: #39006
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 15, 2021
stream: fix pipeline pump
c20e28e 
Refs: #39005

PR-URL: #39006
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams mentioned this pull request Jun 15, 2021
Merged
danielleadams pushed a commit that referenced this pull request Jun 17, 2021
stream: fix pipeline pump
8dc8610 
Refs: #39005

PR-URL: #39006
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

richardlau commented Jul 19, 2021

Copy link
Copy Markdown
Member

This doesn't land cleanly on v14.x-staging.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aduh95 aduh95 left review comments
RaisinTen RaisinTen left review comments
mcollina mcollina approved these changes
jasnell jasnell approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. needs-ci PRs that need a full CI run. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL