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

doc: add pipeline and finished to stream/promises doc #45832

Merged
merged 8 commits into from
Dec 15, 2022
Merged

doc: add pipeline and finished to stream/promises doc #45832

merged 8 commits into from
Dec 15, 2022

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Dec 12, 2022 •
edited by aduh95
Loading

I've added to the doc the signature of pipeline and finished which was missing.
@ronag I've also added the end field which was mentioned here #34805 (comment) but I'm not sure if that's all right.

Fixes: #45821

@nodejs-github-bot nodejs-github-bot added dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. labels Dec 12, 2022
@marco-ippolito marco-ippolito changed the title streams: add pipeline and finished to stream/promises doc doc: add pipeline and finished to stream/promises doc Dec 12, 2022
@marco-ippolito marco-ippolito force-pushed the doc/stream-promises branch 3 times, most recently from acb6aa0 to 0eee704 Compare December 12, 2022 18:59
aduh95
aduh95 reviewed Dec 13, 2022
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Can you add back link to the callback methods and add link this this methods from the callback docs? Adding code examples would be great also.

doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
@marco-ippolito
Copy link
Member Author

I've moved the async examples under the new async documentation and created links

aduh95
aduh95 reviewed Dec 14, 2022
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Looks good. Let's use top-level await on ESM when possible.

doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
aduh95
aduh95 approved these changes Dec 15, 2022
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@marco-ippolito @aduh95
doc: fix setImmediate
083825e 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 15, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/45832
✔  Done loading data for nodejs/node/pull/45832
----------------------------------- PR info ------------------------------------
Title      doc: add  pipeline and finished to stream/promises doc (#45832)
Author     Marco Ippolito  (@marco-ippolito)
Branch     marco-ippolito:doc/stream-promises -> nodejs:main
Labels     author ready, needs-ci, dependencies, commit-queue-squash
Commits    6
 - doc: add stream/promises pipeline and finished to doc
 - doc: fixed correct return type
 - doc: improved documentation
 - doc: typo removed >
 - doc: top level await and typo
 - doc: fix setImmediate
Committers 2
 - Marco Ippolito 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/45832
Fixes: https://github.com/nodejs/node/issues/45821
Reviewed-By: Paolo Insogna 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45832
Fixes: https://github.com/nodejs/node/issues/45821
Reviewed-By: Paolo Insogna 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - doc: fix setImmediate
   ℹ  This PR was created on Mon, 12 Dec 2022 18:51:35 GMT
   ✔  Approvals: 2
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/45832#pullrequestreview-1217021378
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/45832#pullrequestreview-1218851209
   ✔  Last GitHub CI successful
   ✖  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3703786198

aduh95
aduh95 reviewed Dec 15, 2022
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Show resolved Hide resolved
marco-ippolito and others added 2 commits December 15, 2022 14:03
@marco-ippolito @aduh95
fix: period instead of :
50bf959 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@marco-ippolito @aduh95
doc: newline
c930116 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95
Copy link
Contributor

aduh95 commented Dec 15, 2022

Not sure why GitHub didn't run CI on the lastest commit, closing and reopening to trigger CI.

@aduh95 aduh95 closed this Dec 15, 2022
@aduh95 aduh95 reopened this Dec 15, 2022
@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 15, 2022
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/45832
✔  Done loading data for nodejs/node/pull/45832
----------------------------------- PR info ------------------------------------
Title      doc: add  pipeline and finished to stream/promises doc (#45832)
Author     Marco Ippolito  (@marco-ippolito)
Branch     marco-ippolito:doc/stream-promises -> nodejs:main
Labels     author ready, needs-ci, dependencies, commit-queue-squash
Commits    8
 - doc: add stream/promises pipeline and finished to doc
 - doc: fixed correct return type
 - doc: improved documentation
 - doc: typo removed >
 - doc: top level await and typo
 - doc: fix setImmediate
 - fix: period instead of :
 - doc: newline
Committers 2
 - Marco Ippolito 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/45832
Fixes: https://github.com/nodejs/node/issues/45821
Reviewed-By: Paolo Insogna 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/45832
Fixes: https://github.com/nodejs/node/issues/45821
Reviewed-By: Paolo Insogna 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Mon, 12 Dec 2022 18:51:35 GMT
   ✔  Approvals: 2
   ✔  - Paolo Insogna (@ShogunPanda): https://github.com/nodejs/node/pull/45832#pullrequestreview-1217021378
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/45832#pullrequestreview-1219295708
   ✔  Last GitHub CI successful
   ✖  No Jenkins CI runs detected
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/3705387087

@aduh95 aduh95 added doc Issues and PRs related to the documentations. commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. dependencies Pull requests that update a dependency file. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 15, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2022
@nodejs-github-bot nodejs-github-bot merged commit 4166d40 into nodejs:main Dec 15, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 4166d40

targos pushed a commit that referenced this pull request Jan 1, 2023
@marco-ippolito @targos
doc: add stream/promises pipeline and finished to doc
b93a967 
PR-URL: #45832
Fixes: #45821
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@marco-ippolito @juanarbol
doc: add stream/promises pipeline and finished to doc
5eb93f1 
PR-URL: #45832
Fixes: #45821
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Merged
@aloisklink aloisklink mentioned this pull request Jul 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@aduh95 aduh95 aduh95 approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The stream/promises API is poorly documented
4 participants
@marco-ippolito @nodejs-github-bot @aduh95 @ShogunPanda