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 stream as context is redundant #35728

Closed
wants to merge 1 commit into from

Conversation

yashLadha
Copy link
Contributor

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

Review requested:

  • @nodejs/streams

@codecov-io
Copy link

codecov-io commented Oct 21, 2020

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.

// being called before last iteration completion.
let reading = false;

// needToClose boolean if iterator needs to be explicitly closed
// Flag for iterator when needs to be explicitly closed
Copy link
Member

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
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

@nodejs-github-bot
Copy link
Collaborator

@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

// being called before last iteration completion.
let reading = false;

// needToClose boolean if iterator needs to be explicitly closed
// Flag for iterator when needs to be explicitly closed.
Copy link
Member

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

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

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.
gireeshpunathil pushed a commit that referenced this pull request Oct 25, 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.

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

landed in 364ac78

targos pushed a commit that referenced this pull request Nov 3, 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.

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
BethGriggs pushed a commit that referenced this pull request Dec 8, 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.

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
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
BethGriggs pushed a commit that referenced this pull request Dec 15, 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.

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
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants