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

doc: clarify explanation of first stream section #4234

Closed
wants to merge 2 commits into from

Conversation

vccortez
Copy link
Contributor

The last sentence of the explanation for the first stream section
seemed a bit confusing. I tried to change the sentence to clarify it.

The last sentence of the explanation for the first stream section
seemed a bit confusing. I tried to change the sentence to clarify it.
@mscdex mscdex added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Dec 10, 2015
@Fishrock123
Copy link
Contributor

cc @Trott

@Trott
Copy link
Member

Trott commented Dec 11, 2015

I think the sentence can just be removed. It doesn't add any value, IMO.

@jasnell
Copy link
Member

jasnell commented Dec 11, 2015

+1 to simply removing that sentence.

@rvagg
Copy link
Member

rvagg commented Dec 14, 2015

+1 to removal, that section could even be turned into a numbered list for more clarity. Having "This document is split up into 3 sections." alone on a line might make it more likely that people will read it (I bet it hardly ever gets read).

Thanks for the PR @vekat, it looks like this will be your first contribution to Node core, welcome!

@vccortez
Copy link
Contributor Author

Hi everybody, thanks for the feedback. @rvagg so what would be the best way to proceed here?

I'm thinking I could locally revert my initial commit (possibly with rebase -i) and implement your suggestions, then force push a new commit to rewrite my branch.

@Trott
Copy link
Member

Trott commented Dec 14, 2015

@vekat You can just make the changes right on top of the commit you already made. Then either you can squash and force push, or just push normally and there will be two commits in this PR. Whoever lands the PR will squash the two commits to one commit, so it's not a big deal if you don't do it.

The last sentence explaining the first stream section was removed as it
didn't add much value to the explanation.

Additionally, the sections were turned into a numbered list to be more
clear about which section is being described, and improve readability.
@vccortez
Copy link
Contributor Author

Thanks for the quick answer @Trott.

@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

LGTM

jasnell pushed a commit that referenced this pull request Jan 15, 2016
The last sentence of the explanation for the first stream section
seemed a bit confusing. I tried to change the sentence to clarify it.

Additionally, the sections were turned into a numbered list to be more
clear about which section is being described, and improve readability.

PR-URL: #4234
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

Landed in 3470574

@jasnell jasnell closed this Jan 15, 2016
jasnell pushed a commit that referenced this pull request Jan 15, 2016
The last sentence of the explanation for the first stream section
seemed a bit confusing. I tried to change the sentence to clarify it.

Additionally, the sections were turned into a numbered list to be more
clear about which section is being described, and improve readability.

PR-URL: #4234
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

Landed in v4.x-staging in 5608267

evanlucas pushed a commit that referenced this pull request Jan 18, 2016
The last sentence of the explanation for the first stream section
seemed a bit confusing. I tried to change the sentence to clarify it.

Additionally, the sections were turned into a numbered list to be more
clear about which section is being described, and improve readability.

PR-URL: #4234
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
The last sentence of the explanation for the first stream section
seemed a bit confusing. I tried to change the sentence to clarify it.

Additionally, the sections were turned into a numbered list to be more
clear about which section is being described, and improve readability.

PR-URL: #4234
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
@vccortez vccortez deleted the patch-2 branch March 10, 2016 00:33
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
The last sentence of the explanation for the first stream section
seemed a bit confusing. I tried to change the sentence to clarify it.

Additionally, the sections were turned into a numbered list to be more
clear about which section is being described, and improve readability.

PR-URL: nodejs#4234
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants