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

Open
wants to merge 1 commit into
base: master
from

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

@nodejs-github-bot nodejs-github-bot commented Oct 21, 2020

Review requested:

@codecov-io
Copy link

@codecov-io 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.

@lpinca
lpinca approved these changes Oct 21, 2020
// 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

This comment has been minimized.

@lpinca

lpinca Oct 21, 2020
Member

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 lpinca commented Oct 21, 2020

The subsystem in commit title should be stream:.

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

@yashLadha yashLadha commented Oct 21, 2020

Updated @lpinca

@ronag
ronag approved these changes Oct 21, 2020
Copy link
Member

@mcollina mcollina left a comment

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.

This comment has been minimized.

@Trott

Trott Oct 21, 2020
Member

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

This comment has been minimized.

@yashLadha

yashLadha Oct 21, 2020
Author Contributor

Made suggested changes 👍

@Trott
Copy link
Member

@Trott 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.
@yashLadha yashLadha force-pushed the yashLadha:improve_stream_from_doc branch from daa264f to 2aa775a Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.