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

doc: fix typos in the stream doc by vsemozhetbyt · Pull Request #7336 · nodejs/node · GitHub

/ node Public

doc: fix typos in the stream doc - #7336

Closed
vsemozhetbyt wants to merge 7 commits into
nodejs:masterfrom
vsemozhetbyt:patch-1
ConversationCommits (7)ChecksFiles changed
Closed

doc: fix typos in the stream doc#7336
vsemozhetbyt wants to merge 7 commits into
nodejs:masterfrom
vsemozhetbyt:patch-1

Conversation

vsemozhetbyt commented Jun 18, 2016

Copy link
Copy Markdown
Contributor
Checklist
  • documentation is changed or added
Affected core subsystem(s)

doc

Description of change

Fix typo in the link to fs method documentation.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 18, 2016

addaleax commented Jun 18, 2016

Copy link
Copy Markdown
Member

While you’re at it, could you also take a look at the ones listed in #6947 (comment) ?

addaleax added the stream Issues and PRs related to the stream subsystem. label Jun 18, 2016

vsemozhetbyt commented Jun 18, 2016

Copy link
Copy Markdown
Contributor Author

Sorry, I am not a pro in the doc format and I've just found many more strange fragments:

...Note: Transform streams provide their own implementation of the [writable._write()]...

...and may also implement the [transform._flush()][stream-._flush] method...

...In addition to new Readable streams switching into flowing mode, pre-v0.10 style streams can be wrapped in a Readable class using the [readable.wrap()][] method...

And many method calls have strange slash in it (i.e. writable.\_write()). I am afraid I could break the doc more than fix it. May be I should close this PR and wait for more well-informed contributor to fix all the typos.

addaleax commented Jun 18, 2016

Copy link
Copy Markdown
Member

May be I should close this PR and wait for more well-informed contributor to fix all the typos.

Nah, no need to close this. If you don’t feel like fixing the other issues, that’s perfectly fine – this LGTM anyway and I see no reason not to merge it.

I am afraid I could break the doc more than fix it.

That’s what reviews are for, so I wouldn’t worry about that.

vsemozhetbyt commented Jun 18, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Fixed another one that I was confident to fix. Is it OK to add this second commit in this same PR this way?

vsemozhetbyt changed the title doc: fix typo in the stream doc doc: fix typos in the stream doc Jun 18, 2016

lpinca commented Jun 19, 2016

Copy link
Copy Markdown
Member

@vsemozhetbyt yes I think that's ok, whoever lands this will squash your commits if necessary.
I think you can do that yourself in this case (squash and force push).

vsemozhetbyt commented Jun 19, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I've tried to fix another three typos from this coment. The first and second ones seem OK (but I am not sure if they really have proper hrefs now). The third I can't fix, sorry. I hope I have not fouled all the PR by the last tries.

addaleax reviewed Jun 19, 2016
View reviewed changes
Comment thread doc/api/stream.md Outdated
In addition to new Readable streams switching into flowing mode,
pre-v0.10 style streams can be wrapped in a Readable class using the
[`readable.wrap()`][] method.
[`readable.wrap()`][stream-_wrap] method.

addaleax Jun 19, 2016 •
edited
Loading

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

This should be [readable.wrap()][stream.wrap()]?

vsemozhetbyt Jun 19, 2016

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

There should be the backticks in the second brackets too?

addaleax Jun 19, 2016

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

In the footer of the file, there is a [stream.wrap()]: #stream_readable_wrap_stream line, so yes, I think so

addaleax commented Jun 19, 2016

Copy link
Copy Markdown
Member

LGTM with a comment

vsemozhetbyt commented Jun 19, 2016

Copy link
Copy Markdown
Contributor Author

Phew) Finally.

vsemozhetbyt commented Jun 19, 2016

Copy link
Copy Markdown
Contributor Author

Should I also replace all the writable.\_write and like ones with writable._write ? There are 35 ones in the source.

addaleax commented Jun 19, 2016

Copy link
Copy Markdown
Member

The backslashes all show up in https://nodejs.org/api/stream.html, right? If so, I’d be in favour of that.

vsemozhetbyt commented Jun 19, 2016

Copy link
Copy Markdown
Contributor Author

Yes, they show up there. So are they somehow OK or they should be deleted?

addaleax commented Jun 19, 2016

Copy link
Copy Markdown
Member

They should be removed, yes.

vsemozhetbyt commented Jun 19, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Done. Hope nothing is broken after this mass replace (I've done it one by one with a check).

addaleax commented Jun 19, 2016

Copy link
Copy Markdown
Member

btw, you can run make -j8 doc-only in your node checkout and look at the doc output directly (e.g. in out/doc/api/stream.html), if that helps you feel more confident about the changes you are making. :)

vsemozhetbyt commented Jun 19, 2016

Copy link
Copy Markdown
Contributor Author

Sorry. I've restored backslashes in headers. Is it OK now?

addaleax commented Jun 19, 2016

Copy link
Copy Markdown
Member

@vsemozhetbyt looks good, yep

jasnell commented Jun 20, 2016

Copy link
Copy Markdown
Member

LGTM

addaleax commented Jun 21, 2016

Copy link
Copy Markdown
Member

Landed in 58a241d, thanks!

addaleax closed this Jun 21, 2016
addaleax pushed a commit that referenced this pull request Jun 21, 2016
doc: fix typos in the stream doc
58a241d 
PR-URL: #7336
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax mentioned this pull request Jun 21, 2016
Closed
2 tasks
vsemozhetbyt deleted the patch-1 branch June 21, 2016 17:05
evanlucas pushed a commit that referenced this pull request Jun 27, 2016
doc: fix typos in the stream doc
cd43946 
PR-URL: #7336
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
doc: fix typos in the stream doc
e7279f1 
PR-URL: #7336
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fishrock123 mentioned this pull request Jun 27, 2016
Merged
Fishrock123 mentioned this pull request Jul 5, 2016
Merged
MylesBorins mentioned this pull request Jul 11, 2016
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

No reviews

Assignees

No one assigned

Labels

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.

6 participants

Back | FazBrowse Home | New Git URL