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

docs,test: add tests and docs for duplex.fromWeb and duplex.toWeb #42738

Merged
merged 5 commits into from
Apr 28, 2022
Merged

docs,test: add tests and docs for duplex.fromWeb and duplex.toWeb #42738

merged 5 commits into from
Apr 28, 2022

Conversation

ErickWendel
Copy link
Member

Improve Code coverage for Duplex.fromWeb and Duplex.toWeb

This also adds examples of how to use Duplex.fromWeb and Duplex.toWeb

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 14, 2022
@ErickWendel ErickWendel force-pushed the erick/improve-code-coverage-for-duplex-web branch 2 times, most recently from 9e6359a to 9ea3dca Compare April 14, 2022 14:00
@ErickWendel ErickWendel changed the title stream/duplex: add tests and docs for duplex.fromWeb and duplex.toWeb stream: add tests and docs for duplex.fromWeb and duplex.toWeb Apr 14, 2022
@ErickWendel ErickWendel force-pushed the erick/improve-code-coverage-for-duplex-web branch from 9ea3dca to 0d50b71 Compare April 14, 2022 14:17
@mscdex
Copy link
Contributor

mscdex commented Apr 14, 2022

I think the subsystem prefix on the commit message would better be represented by doc,test instead of stream.

@ErickWendel ErickWendel changed the title stream: add tests and docs for duplex.fromWeb and duplex.toWeb docs,test: add tests and docs for duplex.fromWeb and duplex.toWeb Apr 14, 2022
@ErickWendel ErickWendel force-pushed the erick/improve-code-coverage-for-duplex-web branch from 0d50b71 to 3a1c6ae Compare April 14, 2022 14:48
@ErickWendel
Copy link
Member Author

I think the subsystem prefix on the commit message would better be represented by doc,test instead of stream.

I liked it! Just changed both commit message and PR name

lpinca
lpinca approved these changes Apr 14, 2022
View reviewed changes
test/parallel/test-stream-duplex.js Outdated Show resolved Hide resolved
test/parallel/test-stream-duplex.js Outdated Show resolved Hide resolved
test/parallel/test-stream-duplex.js Outdated Show resolved Hide resolved
test/parallel/test-stream-duplex.js Outdated Show resolved Hide resolved
Mesteery
Mesteery approved these changes Apr 14, 2022
View reviewed changes
doc/api/stream.md Outdated Show resolved Hide resolved
@ErickWendel
all small nits suggestions
d248554 
Signed-off-by: Erick Wendel <erick.workspace@gmail.com>
@Trott
Copy link
Member

Trott commented Apr 15, 2022

Should there be CJS examples too so that people have the toggle switch in the docs? It looks like we haven't been doing that in stream.md but you can look at lots of other docs (like crypto.md) for examples and see what it looks like on the website:

image

@ErickWendel
add example in cjs to enable toggle docs
c7f95ff 
Signed-off-by: Erick Wendel <erick.workspace@gmail.com>
@ErickWendel
Copy link
Member Author

Should there be CJS examples too so that people have the toggle switch in the docs? It looks like we haven't been doing that in stream.md but you can look at lots of other docs (like crypto.md) for examples and see what it looks like on the website:

image

Nice! I didn't know that. Just added it!

@Trott Trott 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. labels Apr 18, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 18, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

aduh95
aduh95 reviewed Apr 19, 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.

Can you please add the node: prefix to align with #42752?

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
ErickWendel and others added 2 commits April 19, 2022 21:10
@ErickWendel @aduh95
Update doc/api/stream.md
d5d117e 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ErickWendel @aduh95
docs: apply suggestions from code review
6e2967e 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ErickWendel
Copy link
Member Author

Can you please add the node: prefix to align with #42752?

done!

@github-actions github-actions bot mentioned this pull request Apr 20, 2022
Open
16 tasks
@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 20, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Apr 21, 2022
Open
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 28, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 28, 2022
@nodejs-github-bot nodejs-github-bot merged commit bc47eb3 into nodejs:master Apr 28, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in bc47eb3

targos pushed a commit that referenced this pull request May 2, 2022
@ErickWendel @targos
doc,test: add tests and docs for duplex.fromWeb and duplex.toWeb
a6e1e7a 
PR-URL: #42738
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos mentioned this pull request May 2, 2022
Merged
@juanarbol
Copy link
Member

Depending of #39134 being backported to v16.x

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

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@aduh95 aduh95 aduh95 approved these changes

@juanarbol juanarbol juanarbol approved these changes

@Mesteery Mesteery Mesteery 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. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@ErickWendel @mscdex @Trott @nodejs-github-bot @juanarbol @jasnell @lpinca @aduh95 @Mesteery @targos