FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: clarify Readable paused/flowing!==object mode by cxw42 · Pull Request #22619 · nodejs/node · GitHub

/ node Public

doc: clarify Readable paused/flowing!==object mode - #22619

Closed
cxw42 wants to merge 1 commit into
nodejs:masterfrom
cxw42:patch-1
ConversationCommits (1)ChecksFiles changed
Closed

doc: clarify Readable paused/flowing!==object mode#22619
cxw42 wants to merge 1 commit into
nodejs:masterfrom
cxw42:patch-1

Conversation

cxw42 commented Aug 31, 2018

Copy link
Copy Markdown
Contributor

  • Clarify that a Readable stream's reading mode (paused vs. flowing)
    is independent of its object mode (object vs. non-object). I am
    relatively new to Node streams, and was briefly confused while
    reading the docs by the two uses of the word "mode".

  • Copyediting: add missing apostrophes; minor grammatical changes

Checklist

nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Aug 31, 2018
addaleax approved these changes Aug 31, 2018
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, this PR is 💯!

Comment thread doc/api/stream.md Outdated

addaleax Aug 31, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Maybe switching to === makes sense, given the “flexible” nature of normal JS equality comparisons?

cxw42 Aug 31, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for your quick review and positive feedback! This change is fine with me --- since I don't know the internals, I didn't want to assume. I will update momentarily. Edit Changed in 432fe1225b1ac8d4812f572357c9dfc0e4582291

vsemozhetbyt commented Aug 31, 2018

Copy link
Copy Markdown
Contributor

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/740/

BridgeAR approved these changes Aug 31, 2018
View reviewed changes

BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM!

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 31, 2018
lundibundi approved these changes Aug 31, 2018
View reviewed changes

lundibundi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. 👍

P.s. seems like source-links in docs are starting to pay off).

Comment thread doc/api/stream.md Outdated

lundibundi Aug 31, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nit: two spaces before 'Therefore'.

cxw42 Aug 31, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good catch - will fix along with the change in response to your comment below.

Comment thread doc/api/stream.md Outdated

lundibundi Aug 31, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hm, wdyt of: 'must be called to make stream emit data'?

cxw42 Aug 31, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can do. However, I have a concern about implying determinism. What about "now the stream can emit 'data' events"? Correct me if I am wrong, but I think calling resume() does not mean that a data event will necessarily be emitted, depending on the state of the stream.

lundibundi Aug 31, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It will be emitted after resume if there is data in the stream and we have just added it there via the above write call. Therefore it should be fine with being deterministic here.

cxw42 Aug 31, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Rebased, and both of your changes applied, in 0bb51b0. Thanks!

doc: clarify Readable paused/flowing!==object mode
0bb51b0 
- Clarify that a `Readable` stream's reading mode (paused vs. flowing)
  is independent of its object mode (object vs. non-object).  I am
  relatively new to Node streams, and was briefly confused while
  reading the docs by the two uses of the word "mode".

- Copyediting: add missing apostrophes; minor grammatical changes

lundibundi commented Aug 31, 2018

Copy link
Copy Markdown
Member

Final CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/751/

mcollina approved these changes Sep 3, 2018
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

vsemozhetbyt reviewed Sep 3, 2018
View reviewed changes
Comment thread doc/api/stream.md
vsemozhetbyt pushed a commit that referenced this pull request Sep 3, 2018
doc: clarify Readable paused/flowing!==object mode
4f70ecc 
- Clarify that a `Readable` stream's reading mode (paused vs. flowing)
  is independent of its object mode (object vs. non-object).  I am
  relatively new to Node streams, and was briefly confused while
  reading the docs by the two uses of the word "mode".

- Copyediting: add missing apostrophes; minor grammatical changes

PR-URL: #22619
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

vsemozhetbyt commented Sep 3, 2018

Copy link
Copy Markdown
Contributor

Landed in 4f70ecc (with some exra spaces deleted).
Thank you!

targos pushed a commit that referenced this pull request Sep 3, 2018
doc: clarify Readable paused/flowing!==object mode
a3e3ae0 
- Clarify that a `Readable` stream's reading mode (paused vs. flowing)
  is independent of its object mode (object vs. non-object).  I am
  relatively new to Node streams, and was briefly confused while
  reading the docs by the two uses of the word "mode".

- Copyediting: add missing apostrophes; minor grammatical changes

PR-URL: #22619
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
cxw42 deleted the patch-1 branch September 3, 2018 16:19
targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
doc: clarify Readable paused/flowing!==object mode
c8cd90d 
- Clarify that a `Readable` stream's reading mode (paused vs. flowing)
  is independent of its object mode (object vs. non-object).  I am
  relatively new to Node streams, and was briefly confused while
  reading the docs by the two uses of the word "mode".

- Copyediting: add missing apostrophes; minor grammatical changes

PR-URL: #22619
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This was referenced Sep 6, 2018
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
BridgeAR BridgeAR approved these changes
trivikr trivikr approved these changes
+2 more reviewers
lundibundi lundibundi approved these changes
vsemozhetbyt vsemozhetbyt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. doc Issues and PRs related to the documentations. 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

Back | FazBrowse Home | New Git URL