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

doc: revise http documentation by TimothyGu · Pull Request #8486 · nodejs/node · GitHub

/ node Public

doc: revise http documentation - #8486

Closed
TimothyGu wants to merge 2 commits into
nodejs:masterfrom
TimothyGu:http-params
ConversationCommits (2)ChecksFiles changed
Closed

doc: revise http documentation#8486
TimothyGu wants to merge 2 commits into
nodejs:masterfrom
TimothyGu:http-params

Conversation

TimothyGu commented Sep 10, 2016

Copy link
Copy Markdown
Member
Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

The current documentation for the http module still follows an older format. Type documentation for parameters were also incomplete. This pull request at least fills up the type information for parameters and returns, so that the parsed JSON is usable.

mscdex added http Issues or PRs related to the http subsystem. doc Issues and PRs related to the documentations. labels Sep 11, 2016

jasnell commented Sep 12, 2016

Copy link
Copy Markdown
Member

@nodejs/documentation

thefourtheye reviewed Sep 12, 2016
View reviewed changes
Comment thread doc/api/http.md Outdated

thefourtheye Sep 12, 2016

Copy link
Copy Markdown
Contributor

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

Strictly speaking, async functions return nothing. Isn't that the case here?

TimothyGu Sep 12, 2016

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

The documentation states:

A socket/stream can be supplied in one of two ways: by returning the socket/stream from this function, or by passing the socket/stream to callback.

So it's not a "true" async or sync function. Indeed, the code explicitly supports both sync and async usage:

  const newSocket = self.createConnection(options, oncreate);
  if (newSocket)
    oncreate(null, newSocket);

thefourtheye Sep 25, 2016

Copy link
Copy Markdown
Contributor

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

Okay, in that case, can we atleast specify that the callback is optional?

TimothyGu Sep 25, 2016

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

It is, as shown in the function signature:

### agent.createConnection(options[, callback])

imyller approved these changes Sep 21, 2016
View reviewed changes

imyller 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

addaleax approved these changes Sep 24, 2016
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

LGTM, thanks for putting in the effort to do this!

lpinca reviewed Sep 25, 2016
View reviewed changes
Comment thread doc/api/http.md Outdated

lpinca Sep 25, 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 is not 100% true, I stumbled upon this the other day. There are cases where socket.setTimeout() is called even if the socket is not connected.
Anyway it's probably better to investigate/fix this in another issue/PR.

lpinca reviewed Sep 25, 2016
View reviewed changes
Comment thread doc/api/http.md Outdated

lpinca Sep 25, 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

This should be 'request'.

lpinca approved these changes Sep 25, 2016
View reviewed changes

lpinca 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

lpinca commented Sep 25, 2016

Copy link
Copy Markdown
Member

I really appreciate the effort that has been put here, thanks.

ChALkeR commented Sep 25, 2016 •
edited
Loading

Copy link
Copy Markdown
Member 
http.md
  1585:1-1585:38  warning  Found unused definition  no-unused-definitions  remark-lint

⚠ 1 warning

This definition became unused:

[`Buffer`]: buffer.html#buffer_buffer

jasnell commented Oct 7, 2016

Copy link
Copy Markdown
Member

Ping @TimothyGu ... can you take a look at @ChALkeR's most recent comment.

doc: revise http documentation
9b5faa8
doc: fix location of 'checkExpectation' event
205bebc 
It is emitted by the Server, analogous to the `'checkContinue'` event,
not by the ClientRequest.

TimothyGu commented Oct 8, 2016

Copy link
Copy Markdown
Member Author

@jasnell, fixed.

TimothyGu commented Oct 11, 2016

Copy link
Copy Markdown
Member Author

Ping?

jasnell commented Oct 11, 2016

Copy link
Copy Markdown
Member

@ChALkeR ... LGTY?

ChALkeR approved these changes Oct 11, 2016
View reviewed changes

ChALkeR 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

jasnell self-assigned this Oct 11, 2016

jasnell commented Oct 11, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell pushed a commit that referenced this pull request Oct 11, 2016
doc: revise http documentation
52c7f9d 
PR-URL: #8486
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Oct 11, 2016

Copy link
Copy Markdown
Member

Landed in 52c7f9d

jasnell closed this Oct 11, 2016
TimothyGu deleted the http-params branch October 12, 2016 00:39
jasnell pushed a commit that referenced this pull request Oct 12, 2016
doc: revise http documentation
8f6589a 
PR-URL: #8486
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
doc: revise http documentation
d312899 
PR-URL: #8486
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins commented Nov 11, 2016

Copy link
Copy Markdown
Contributor

I've landed this in v6.x-staging. Please let me know if there is anything in here that is specific to v7 and should not be included

TimothyGu mentioned this pull request Nov 12, 2016
Closed
2 tasks
MylesBorins mentioned this pull request Nov 22, 2016
Merged
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

ChALkeR ChALkeR approved these changes
addaleax addaleax approved these changes
lpinca lpinca approved these changes
+2 more reviewers
thefourtheye thefourtheye left review comments
imyller imyller approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

jasnell

Labels

doc Issues and PRs related to the documentations. http Issues or PRs related to the http 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