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.

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

lib: fix duplicate logic in stream destroy #35727

Closed

Conversation

yashLadha
Copy link
Contributor

Fix duplicate logic in stream destroy as the same logic is being shared
across methods and thus can be encapsulated into a single method.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@codecov-io
Copy link

Codecov Report

Merging #35727 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #35727      +/-   ##
==========================================
- Coverage   96.40%   96.40%   -0.01%     
==========================================
  Files         222      223       +1     
  Lines       73682    73679       -3     
==========================================
- Hits        71033    71029       -4     
- Misses       2649     2650       +1     
Impacted Files Coverage Δ
lib/internal/streams/destroy.js 96.80% <100.00%> (-0.06%) ⬇️
lib/_http_server.js 98.34% <0.00%> (-0.11%) ⬇️
lib/util/types.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f296d2...fb42577. Read the comment docs.

@lpinca
Copy link
Member

lpinca commented Oct 21, 2020

The subsystem in commit title should be stream:.

Fix duplicate logic in stream destroy as the same logic is being shared
across methods and thus can be encapsulated into a single method.
@yashLadha yashLadha force-pushed the fix_duplicate_logic_stream_destroy branch from fb42577 to 464d7c6 Compare October 21, 2020 05:36
@yashLadha
Copy link
Contributor Author

Updated @lpinca

@gireeshpunathil gireeshpunathil added request-ci Add this label to start a Jenkins CI on a PR. stream Issues and PRs related to the stream subsystem. labels Oct 21, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 21, 2020
@nodejs-github-bot
Copy link
Collaborator

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
Copy link
Collaborator

@gireeshpunathil gireeshpunathil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 21, 2020
gireeshpunathil pushed a commit that referenced this pull request Oct 24, 2020
Fix duplicate logic in stream destroy as the same logic is being shared
across methods and thus can be encapsulated into a single method.

PR-URL: #35727
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@gireeshpunathil
Copy link
Member

landed in 4d54449

@yashLadha yashLadha deleted the fix_duplicate_logic_stream_destroy branch October 24, 2020 08:14
targos pushed a commit that referenced this pull request Nov 3, 2020
Fix duplicate logic in stream destroy as the same logic is being shared
across methods and thus can be encapsulated into a single method.

PR-URL: #35727
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@targos targos mentioned this pull request Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.