Skip to content
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.

Sign up for GitHub

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

Jump to bottom

stream: add isErrored helper #41121

Closed
wants to merge 6 commits into from
Closed

stream: add isErrored helper #41121

wants to merge 6 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 9, 2021

@ronag ronag added stream Issues and PRs related to the stream subsystem. web streams labels Dec 9, 2021
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 9, 2021
@ronag
Copy link
Member Author

ronag commented Dec 9, 2021

@nodejs/streams

@ronag ronag mentioned this pull request Dec 9, 2021
Merged
mcollina
mcollina approved these changes Dec 9, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag force-pushed the stream-is-errored branch 2 times, most recently from c76beb4 to 1e2413b Compare December 9, 2021 09:33
@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Dec 9, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 9, 2021
@nodejs-github-bot
Copy link
Collaborator

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

lpinca
lpinca reviewed Dec 9, 2021
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Dec 9, 2021
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
mscdex
mscdex reviewed Dec 9, 2021
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@mscdex
Copy link
Contributor

mscdex commented Dec 9, 2021

Is there a reason this is only for Readable streams?

Also, maybe we should name it hasErrored() or hasError() instead?

ronag
ronag commented Dec 9, 2021
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
ronag and others added 2 commits December 9, 2021 15:34
@ronag @lpinca @mscdex
Apply suggestions from code review
47b2605 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
@ronag @mscdex
Update doc/api/stream.md
571739e 
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
ronag
ronag commented Dec 9, 2021
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@ronag ronag requested review from lpinca and mscdex December 9, 2021 14:34
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 14, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 14, 2021
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Dec 15, 2021
Open
Open
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 16, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag mentioned this pull request Dec 16, 2021
Closed
@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Dec 16, 2021

Landed in 752d75d

ronag added a commit that referenced this pull request Dec 16, 2021
@ronag
stream: add isErrored helper
752d75d 
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@ronag ronag closed this Dec 16, 2021
@ronag ronag mentioned this pull request Dec 16, 2021
Merged
@lpinca
Copy link
Member

lpinca commented Dec 16, 2021

It's a bit too late but I agree with @mscdex about the name. hasErrored() seems more correct.

danielleadams pushed a commit that referenced this pull request Dec 16, 2021
@ronag @danielleadams
stream: add isErrored helper
9b0cafd 
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@github-actions github-actions bot mentioned this pull request Dec 17, 2021
Open
22 tasks
danielleadams pushed a commit that referenced this pull request Dec 17, 2021
@ronag @danielleadams
stream: add isErrored helper
b59c513 
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
This was referenced Dec 18, 2021
Open
Open
Open
Open
Open
danielleadams pushed a commit that referenced this pull request Jan 31, 2022
@ronag @danielleadams
stream: add isErrored helper
2568ac9 
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
@ronag @Linkgoron
stream: add isErrored helper
b7c0d6c 
Refs: nodejs/undici#1134

PR-URL: nodejs#41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@ronag @danielleadams
stream: add isErrored helper
4522216 
Refs: nodejs/undici#1134

PR-URL: #41121
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aymen94 aymen94 aymen94 approved these changes

@JungMinu JungMinu JungMinu approved these changes

@mcollina mcollina mcollina approved these changes

@jasnell jasnell Awaiting requested review from jasnell

@lpinca lpinca Awaiting requested review from lpinca

@mscdex mscdex Awaiting requested review from mscdex

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem. web streams
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ronag @nodejs-github-bot @mscdex @lpinca @mcollina @JungMinu @aymen94