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: refactor streams #41871

Closed
wants to merge 3 commits into from
Closed

stream: refactor streams #41871

wants to merge 3 commits into from

Conversation

VoltrexKeyva
Copy link
Member

@VoltrexKeyva VoltrexKeyva commented Feb 6, 2022 •
edited
Loading

  • Use the standard for loop style instead of for..of for consistency.
  • Refactor to use more validators.
  • Avoid using deprecated APIs.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 6, 2022
@VoltrexKeyva VoltrexKeyva added the stream Issues and PRs related to the stream subsystem. label Feb 6, 2022
mcollina
mcollina approved these changes Feb 6, 2022
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

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@aduh95
Copy link
Contributor

aduh95 commented Feb 6, 2022

Can we use a less scary commit message? 😅

@VoltrexKeyva
Copy link
Member Author

VoltrexKeyva commented Feb 6, 2022 •
edited
Loading

Can we use a less scary commit message? 😅

I'm unsure what you mean by that, do you mean the commit message title? If so, what do you suggest we use instead? (Or if that was a joke :^))

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Feb 6, 2022

If so, what do you suggest we use instead?

Regarding the commit title, I suggest: stream: refactor to use more validators and avoid using deprecated APIs.

refactor streams is a bit too broad, and could imply the commit is doing a complete refactor of the stream API.

aduh95
aduh95 approved these changes Feb 6, 2022
View reviewed changes
lib/internal/streams/duplex.js Outdated Show resolved Hide resolved
@VoltrexKeyva
Copy link
Member Author

If so, what do you suggest we use instead?

Regarding the commit title, I suggest: stream: refactor to use more validators and avoid using deprecated APIs.

refactor streams is a bit too broad, and could imply the commit is doing a complete refactor of the stream API.

That exceeds the 50 character limit for commit message titles (because of our linters), and these changes touch upon multiple files, which is why I chose to use refactor streams, I don't think there's a replacement for the current commit message title that explains the changes without exceeding the character limit?

@aduh95
Copy link
Contributor

aduh95 commented Feb 6, 2022

That exceeds the 50 character limit for commit message titles (because of our linters)

It doesn't exceed our 72 character limit so we're good. Another (better?) solution would be to split this into three separate commits.

@Mesteery
Copy link
Contributor

Mesteery commented Feb 6, 2022 •
edited
Loading

Maybe you can split this commit into 3 commits (+ commit-queue-rebase), for example:

  • stream: refactor to use more validators
  • stream: use for loop instead of for-of loop (including the block statement removal and aduh's suggestion)
  • stream: replace the use of static listenerCount

(I'm too slow 👀😄)

@Mesteery Mesteery closed this Feb 6, 2022
@Mesteery Mesteery reopened this Feb 6, 2022
@github-actions github-actions bot mentioned this pull request Feb 7, 2022
Open
29 tasks
@VoltrexKeyva VoltrexKeyva force-pushed the refactor-streams branch 2 times, most recently from 1251528 to 57e292d Compare February 7, 2022 07:41
nodejs-github-bot pushed a commit that referenced this pull request Feb 11, 2022
@VoltrexKeyva @nodejs-github-bot
stream: use standard for loop instead of for..of
ad6cd7f 
Use the standard `for` loop style instead of `for..of` for
consistency.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Feb 11, 2022
@VoltrexKeyva @nodejs-github-bot
stream: refactor to use more validators
717de17 
Use more validators where appropriate for consistency.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Feb 11, 2022
@VoltrexKeyva @nodejs-github-bot
stream: avoid usage of deprecated APIs
849fb70 
Avoid usage of the `events.listenerCount()` method as it is
deprecated.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@VoltrexKeyva VoltrexKeyva deleted the refactor-streams branch February 11, 2022 11:52
This was referenced Feb 12, 2022
Open
Open
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@VoltrexKeyva @bengl
stream: use standard for loop instead of for..of
7f69973 
Use the standard `for` loop style instead of `for..of` for
consistency.

PR-URL: nodejs#41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@VoltrexKeyva @bengl
stream: refactor to use more validators
308d31d 
Use more validators where appropriate for consistency.

PR-URL: nodejs#41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@VoltrexKeyva @bengl
stream: avoid usage of deprecated APIs
e57267b 
Avoid usage of the `events.listenerCount()` method as it is
deprecated.

PR-URL: nodejs#41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@VoltrexKeyva @bengl
stream: use standard for loop instead of for..of
251279c 
Use the standard `for` loop style instead of `for..of` for
consistency.

PR-URL: nodejs#41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@VoltrexKeyva @bengl
stream: refactor to use more validators
a4b1a09 
Use more validators where appropriate for consistency.

PR-URL: nodejs#41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
bengl pushed a commit to bengl/node that referenced this pull request Feb 21, 2022
@VoltrexKeyva @bengl
stream: avoid usage of deprecated APIs
be7cc04 
Avoid usage of the `events.listenerCount()` method as it is
deprecated.

PR-URL: nodejs#41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@bengl bengl mentioned this pull request Feb 21, 2022
Merged
bengl pushed a commit that referenced this pull request Feb 22, 2022
@VoltrexKeyva @bengl
stream: use standard for loop instead of for..of
07463b3 
Use the standard `for` loop style instead of `for..of` for
consistency.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
bengl pushed a commit that referenced this pull request Feb 22, 2022
@VoltrexKeyva @bengl
stream: refactor to use more validators
5195479 
Use more validators where appropriate for consistency.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
bengl pushed a commit that referenced this pull request Feb 22, 2022
@VoltrexKeyva @bengl
stream: avoid usage of deprecated APIs
3e38506 
Avoid usage of the `events.listenerCount()` method as it is
deprecated.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@danielleadams
Copy link
Member

@VoltrexMaster this doesn't land cleanly in 16.x - do you mind making a backport PR?

@VoltrexKeyva
Copy link
Member Author

@VoltrexMaster this doesn't land cleanly in 16.x - do you mind making a backport PR?

What's blocking it from landing cleanly? I don't see anything that should be changed to land on v16.x

targos pushed a commit that referenced this pull request Jul 12, 2022
@VoltrexKeyva @targos
stream: use standard for loop instead of for..of
07f9822 
Use the standard `for` loop style instead of `for..of` for
consistency.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
@VoltrexKeyva @targos
stream: refactor to use more validators
02c407c 
Use more validators where appropriate for consistency.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
@VoltrexKeyva @targos
stream: avoid usage of deprecated APIs
3da7ea0 
Avoid usage of the `events.listenerCount()` method as it is
deprecated.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@VoltrexKeyva @targos
stream: use standard for loop instead of for..of
545b2cd 
Use the standard `for` loop style instead of `for..of` for
consistency.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@VoltrexKeyva @targos
stream: refactor to use more validators
4ad5a28 
Use more validators where appropriate for consistency.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
@VoltrexKeyva @targos
stream: avoid usage of deprecated APIs
3bc12c6 
Avoid usage of the `events.listenerCount()` method as it is
deprecated.

PR-URL: #41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@VoltrexKeyva @guangwong
stream: use standard for loop instead of for..of
806a212 
Use the standard `for` loop style instead of `for..of` for
consistency.

PR-URL: nodejs/node#41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@VoltrexKeyva @guangwong
stream: refactor to use more validators
3aa1d99 
Use more validators where appropriate for consistency.

PR-URL: nodejs/node#41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@VoltrexKeyva @guangwong
stream: avoid usage of deprecated APIs
5ae33d6 
Avoid usage of the `events.listenerCount()` method as it is
deprecated.

PR-URL: nodejs/node#41871
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@ronag ronag ronag approved these changes

@aduh95 aduh95 aduh95 approved these changes

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@JungMinu JungMinu JungMinu approved these changes

@RaisinTen RaisinTen RaisinTen 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-rebase Add this label to allow the Commit Queue to land a PR in several commits. 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.