doc: fix some mistakes of http-api-doc by MoonBall · Pull Request #14625 · nodejs/node

MoonBall · 2017-08-04T10:02:54Z

fix some mistakes of http-api-doc.

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc, http

MoonBall · 2017-08-04T10:07:59Z

doc/api/http.md

Agent's pool is agent.freeSockets. When a socket is assigned to a request, the socket must not be in agent.freeSockets.

MoonBall · 2017-08-04T10:09:37Z

doc/api/http.md

The req in the code is not http.ClientRequest.

vsemozhetbyt · 2017-08-04T15:28:33Z

Thank you.
It seems this branch needs to be rebased to include the recent changes for the last eslint-plugin-markdown version (15599cb), otherwise, we may get linter CI fail.

vsemozhetbyt · 2017-08-04T15:29:58Z

doc/api/http.md

ESLint error:

599:22 error Expected parentheses around arrow function argument arrow-parens

vsemozhetbyt · 2017-08-04T15:31:58Z

doc/api/http.md

This line exceeds 80 characters and needs to be rewrapped.

vsemozhetbyt · 2017-08-04T15:35:37Z

doc/api/http.md

This link needs a reference at the bottom of the doc.

vsemozhetbyt · 2017-08-04T15:36:34Z

doc/api/http.md

This link needs a reference at the bottom of the doc.

vsemozhetbyt · 2017-08-04T15:40:22Z

cc @nodejs/http

MoonBall · 2017-08-06T05:12:19Z

@vsemozhetbyt what should I do if I want to know these errors in my machine before pushing a new commit.

vsemozhetbyt · 2017-08-06T09:02:51Z

@MoonBall

80 characters errors: we plan using markdown linting in the near future; till that time you can try to make your editor somehow signal the edge (some editors can draw a border on 80 characters margin).
Links errors: just open the rendered version of the doc and see how your new links are rendered. You can also use browser's "search on the page" to find markdown source code artifacts in a rendered text (for example ][] fragments for not-rendered links).
ESLint errors in code fragments (we use eslint-plugin-markdown for that): just rebase your branch to update it and run this command from repository root folder:

node tools\eslint\bin\eslint.js --rulesdir tools/eslint-rules/ --ext=.js,.md benchmark doc lib test tools

Feel free to ask any more questions if I am unclear on something)

MoonBall · 2017-08-06T09:43:45Z

@vsemozhetbyt How can I get the rendered version of the doc?

vsemozhetbyt · 2017-08-06T09:48:58Z

@MoonBall You can click on the "View" button from the diff tab (I've marked it by the red line):

vsemozhetbyt · 2017-08-06T09:55:09Z

Or, if you will want to check errors before creating a PR, you can view a doc from your new pushed branch in your fork:

https://github.com/MoonBall/node/blob/modify-http-api-doc/doc/api/http.md

MoonBall · 2017-08-06T10:16:54Z

@vsemozhetbyt Thank you. I have fixed these problems.

vsemozhetbyt · 2017-08-06T10:24:42Z

@MoonBall Thank you!
Formal aspect LGTM.
HTTP semantics changes need to be reviewed by someone from @nodejs/http team.

vsemozhetbyt · 2017-08-06T10:27:00Z

Linter CI: https://ci.nodejs.org/job/node-test-linter/10974/ (green).

MoonBall · 2017-08-11T06:43:23Z

@vsemozhetbyt The linter CI seems like disappering after I push a new commit by using --force. >.<

MoonBall · 2017-08-11T08:38:43Z

@vsemozhetbyt I find a good way to get the rendered doc from Building Node.js. Just run NODE=/path/to/node make doc-only and open the html file in ./out/doc/api/.

vsemozhetbyt · 2017-08-11T09:03:13Z

The linter CI seems like disappering after I push a new commit by using --force

This should work this way, it is OK.

For the future: if you add some changes after a review, it is better to do this in an additional commit (it will be squashed later), so the reviewers should not re-review all the diff again.

Could you enumerate the new changes in the amended commit?

Linter CI: https://ci.nodejs.org/job/node-test-linter/11107/ (green).

MoonBall · 2017-08-11T09:49:12Z

OK. I comment the new changed code in the commit.

MoonBall · 2017-08-11T09:51:41Z

doc/api/http.md

-  const ip = req.socket.remoteAddress;
-  const port = req.socket.remotePort;
+  const ip = res.socket.remoteAddress;
+  const port = res.socket.remotePort;


@jasnell @vsemozhetbyt It's changed after you reviewed.

MoonBall · 2017-08-11T09:52:08Z

doc/api/http.md

 -->

-* `request` {http.ClientRequest}
+* `request` {http.IncomingMessage}


@jasnell @vsemozhetbyt It's changed after you reviewed.

vsemozhetbyt · 2017-08-18T09:36:23Z

@jasnell Do the last changes seem OK for you? Can we land?

PR-URL: #14625 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

vsemozhetbyt · 2017-08-18T14:14:46Z

Landed in 4d842e3

PR-URL: #14625 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Aug 4, 2017

MoonBall commented Aug 4, 2017

View reviewed changes

doc/api/http.md Outdated

Copy link

Member Author

MoonBall Aug 4, 2017

Choose a reason for hiding this comment

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

The req in the code is not http.ClientRequest.

vsemozhetbyt reviewed Aug 4, 2017

View reviewed changes

MoonBall force-pushed the modify-http-api-doc branch from 592f69b to 2e05b30 Compare August 6, 2017 05:09

MoonBall force-pushed the modify-http-api-doc branch from 2e05b30 to ee11a57 Compare August 6, 2017 10:13

jasnell approved these changes Aug 8, 2017

View reviewed changes

doc: fix http api document

35fc4f2

MoonBall force-pushed the modify-http-api-doc branch from ee11a57 to 35fc4f2 Compare August 10, 2017 12:06

MoonBall commented Aug 11, 2017

View reviewed changes

jasnell approved these changes Aug 18, 2017

View reviewed changes

vsemozhetbyt pushed a commit that referenced this pull request Aug 18, 2017

doc: fix http api document

4d842e3

PR-URL: #14625 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

vsemozhetbyt closed this Aug 18, 2017

MylesBorins pushed a commit that referenced this pull request Sep 10, 2017

doc: fix http api document

639e8d4

PR-URL: #14625 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins pushed a commit that referenced this pull request Sep 12, 2017

doc: fix http api document

abe0148

PR-URL: #14625 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

Uh oh!

Conversation

MoonBall commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Aug 4, 2017

Uh oh!

MoonBall commented Aug 6, 2017

Uh oh!

vsemozhetbyt commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoonBall commented Aug 6, 2017

Uh oh!

vsemozhetbyt commented Aug 6, 2017

Uh oh!

vsemozhetbyt commented Aug 6, 2017

Uh oh!

MoonBall commented Aug 6, 2017

Uh oh!

vsemozhetbyt commented Aug 6, 2017

Uh oh!

vsemozhetbyt commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoonBall commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoonBall commented Aug 11, 2017

Uh oh!

vsemozhetbyt commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoonBall commented Aug 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Aug 18, 2017

Uh oh!

vsemozhetbyt commented Aug 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MoonBall commented Aug 4, 2017 •

edited

Loading

vsemozhetbyt commented Aug 6, 2017 •

edited

Loading

vsemozhetbyt commented Aug 6, 2017 •

edited

Loading

MoonBall commented Aug 11, 2017 •

edited

Loading

vsemozhetbyt commented Aug 11, 2017 •

edited

Loading