Skip to content

lib: fix stream as context is redundant#35728

Closed
yashLadha wants to merge 1 commit intonodejs:masterfrom
yashLadha:improve_stream_from_doc
Closed

lib: fix stream as context is redundant#35728
yashLadha wants to merge 1 commit intonodejs:masterfrom
yashLadha:improve_stream_from_doc

Conversation

@yashLadha
Copy link
Contributor

@yashLadha yashLadha commented Oct 21, 2020

Using the variable name in the comment and justifying the type seems
redundant to me and instead it should defined the entity which it is
acting, like in our case it is acting as a flag to control the flow in
streams.

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

nodejs-github-bot commented Oct 21, 2020

Review requested:

  • @nodejs/streams

@codecov-io
Copy link

codecov-io commented Oct 21, 2020 •
edited
Loading

Codecov Report

Merging #35728 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #35728   +/-   ##
=======================================
  Coverage   96.40%   96.40%           
=======================================
  Files         222      223    +1     
  Lines       73682    73685    +3     
=======================================
+ Hits        71033    71036    +3     
  Misses       2649     2649
Impacted Files Coverage Δ
lib/internal/streams/from.js 98.01% <100.00%> (ø)
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...2aa775a. Read the comment docs.

lpinca
lpinca approved these changes Oct 21, 2020
View reviewed changes
lib/internal/streams/from.js Outdated
Copy link
Member

@lpinca lpinca Oct 21, 2020

Choose a reason for hiding this comment

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

Suggested change
// Flag for iterator when needs to be explicitly closed
// Flag for iterator when needs to be explicitly closed.

@lpinca
Copy link
Member

lpinca commented Oct 21, 2020

The subsystem in commit title should be stream:.

@yashLadha yashLadha force-pushed the improve_stream_from_doc branch from 043cd17 to daa264f Compare October 21, 2020 05:37
@yashLadha
Copy link
Contributor Author

yashLadha commented Oct 21, 2020

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

nodejs-github-bot commented Oct 21, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 21, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 21, 2020

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

mcollina
mcollina approved these changes Oct 21, 2020
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

Trott
Trott reviewed Oct 21, 2020
View reviewed changes
lib/internal/streams/from.js Outdated
Copy link
Member

@Trott Trott Oct 21, 2020

Choose a reason for hiding this comment

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

Suggested change
// Flag for iterator when needs to be explicitly closed.
// Flag for when iterator needs to be explicitly closed.

Copy link
Contributor Author

@yashLadha yashLadha Oct 21, 2020

Choose a reason for hiding this comment

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

Made suggested changes 👍

@Trott
Copy link
Member

Trott commented Oct 21, 2020

The commit message (stream: fix stream as context is redundant ) is unclear to me. Maybe this? stream: remove redundant context from comments

@yashLadha
stream: remove redundant context from comments
2aa775a 
Using the variable name in the comment and justifying the type seems
redundant to me and instead it should defined the entity which it is
acting, like in our case it is acting as a flag to control the flow in
streams.
@yashLadha yashLadha force-pushed the improve_stream_from_doc branch from daa264f to 2aa775a Compare October 21, 2020 16:09
gireeshpunathil pushed a commit that referenced this pull request Oct 25, 2020
@yashLadha @gireeshpunathil
stream: remove redundant context from comments
364ac78 
Using the variable name in the comment and justifying the type seems
redundant to me and instead it should defined the entity which it is
acting, like in our case it is acting as a flag to control the flow in
streams.

PR-URL: #35728
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
@gireeshpunathil
Copy link
Member

gireeshpunathil commented Oct 25, 2020

landed in 364ac78

targos pushed a commit that referenced this pull request Nov 3, 2020
@yashLadha @targos
stream: remove redundant context from comments
70c41a8 
Using the variable name in the comment and justifying the type seems
redundant to me and instead it should defined the entity which it is
acting, like in our case it is acting as a flag to control the flow in
streams.

PR-URL: #35728
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
@targos targos mentioned this pull request Nov 3, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 8, 2020
@yashLadha @BethGriggs
stream: remove redundant context from comments
1723712 
Using the variable name in the comment and justifying the type seems
redundant to me and instead it should defined the entity which it is
acting, like in our case it is acting as a flag to control the flow in
streams.

PR-URL: #35728
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@yashLadha @BethGriggs
stream: remove redundant context from comments
b74f093 
Using the variable name in the comment and justifying the type seems
redundant to me and instead it should defined the entity which it is
acting, like in our case it is acting as a flag to control the flow in
streams.

PR-URL: #35728
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@yashLadha @BethGriggs
stream: remove redundant context from comments
d1fd3f2 
Using the variable name in the comment and justifying the type seems
redundant to me and instead it should defined the entity which it is
acting, like in our case it is acting as a flag to control the flow in
streams.

PR-URL: #35728
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments

@mcollina mcollina mcollina approved these changes

@lpinca lpinca lpinca approved these changes

@ronag ronag ronag approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+1 more reviewer

@antsmartian antsmartian antsmartian approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

9 participants

@yashLadha @nodejs-github-bot @codecov-io @lpinca @Trott @gireeshpunathil @mcollina @antsmartian @ronag