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: fix typos in the stream doc #7336

Closed
wants to merge 7 commits into from
Closed

doc: fix typos in the stream doc #7336

wants to merge 7 commits into from

Conversation

vsemozhetbyt
Copy link
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 nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 18, 2016
@addaleax
Copy link
Member

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

@addaleax addaleax added the stream Issues and PRs related to the stream subsystem. label Jun 18, 2016
@vsemozhetbyt
Copy link
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
Copy link
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
Copy link
Contributor Author

vsemozhetbyt commented Jun 18, 2016

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

@vsemozhetbyt vsemozhetbyt changed the title doc: fix typo in the stream doc doc: fix typos in the stream doc Jun 18, 2016
@lpinca
Copy link
Member

lpinca commented Jun 19, 2016

@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
Copy link
Contributor Author

vsemozhetbyt commented Jun 19, 2016

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.

@@ -1848,7 +1848,7 @@ net.createServer((socket) => {

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.
Copy link
Member

@addaleax addaleax Jun 19, 2016

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be the backticks in the second brackets too?

Copy link
Member

Choose a reason for hiding this comment

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

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

@addaleax
Copy link
Member

LGTM with a comment

@vsemozhetbyt
Copy link
Contributor Author

Phew) Finally.

@vsemozhetbyt
Copy link
Contributor Author

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

@addaleax
Copy link
Member

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

@vsemozhetbyt
Copy link
Contributor Author

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

@addaleax
Copy link
Member

They should be removed, yes.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Jun 19, 2016

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

@addaleax
Copy link
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
Copy link
Contributor Author

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

@addaleax
Copy link
Member

@vsemozhetbyt looks good, yep

@jasnell
Copy link
Member

jasnell commented Jun 20, 2016

LGTM

@addaleax
Copy link
Member

Landed in 58a241d, thanks!

@addaleax addaleax closed this Jun 21, 2016
addaleax pushed a commit that referenced this pull request Jun 21, 2016
PR-URL: #7336
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax mentioned this pull request Jun 21, 2016
2 tasks
@vsemozhetbyt vsemozhetbyt deleted the patch-1 branch June 21, 2016 17:05
evanlucas pushed a commit that referenced this pull request Jun 27, 2016
PR-URL: #7336
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
PR-URL: #7336
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Jun 27, 2016
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
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