Skip to content

doc,http2: add parameters for Http2Session:connect event - #20193

Closed
ryzokuken wants to merge 1 commit into
nodejs:masterfrom
ryzokuken:docs/http2-1
Closed

doc,http2: add parameters for Http2Session:connect event#20193
ryzokuken wants to merge 1 commit into
nodejs:masterfrom
ryzokuken:docs/http2-1

Conversation

@ryzokuken

Copy link
Copy Markdown
Contributor

Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.

Refs: nodejs/help#877 (comment)

Checklist

/cc @mcollina @nodejs/http2

@ryzokukenryzokuken self-assigned this Apr 21, 2018
@ryzokuken
ryzokuken requested a review from mcollinaApril 21, 2018 05:40
@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Apr 21, 2018
@Trott

Copy link
Copy Markdown
Member

We don't typically list parameters like that for events, do we? I think it's useful information to have, but I'm not sure that's the best way to list it. (Maybe it is. I honestly don't know.)

@nodejs/documentation

@apapirovski

Copy link
Copy Markdown
Contributor

@Trott we do in the http and streams documentation. For example: https://nodejs.org/dist/latest-v9.x/docs/api/http.html#http_event_connect

@Trott

Copy link
Copy Markdown
Member

@Trott we do in the http and streams documentation. For example: https://nodejs.org/dist/latest-v9.x/docs/api/http.html#http_event_connect

I never noticed that. OK, cool. If it's what we already do elsewhere, at least it's a unified approach.

@TimothyGuTimothyGu 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.

I don't think the "The associated ... instance" is that helpful since the same information is conveyed by the type annotation already. Otherwise, LGTM.

Comment threaddoc/api/http2.md Outdated

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.

You can just use {Http2Session} here.

@mcollinamcollina 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.

LGTM with @TimothyGu nit.

@ryzokuken

Copy link
Copy Markdown
ContributorAuthor

@TimothyGu@mcollina is this better?

Comment threaddoc/api/http2.md Outdated

@vsemozhetbytvsemozhetbytApr 21, 2018

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.

It's a bit unclear what this line means. If this is the first parameter of an event listener, we need to add a name to it, as the only unnamed types in docs are types of properties and returned values. If this is an event emitter, I do not think we formally state them in this way, as this is already inferred from the section place; this can baffle doctools.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Umm, so should I add a name to it or skip it?

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.

If this is not a listener parameter, but an event source, I think we can skip it as this event is already a subsection of Http2Session section.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

It's a parameter. Code:

process.nextTick(emit,this,'connect',this,socket);

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.

So we need to add a name then for consistency if I am not mistaken. It seems we normally have not unnamed parameters in docs.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

naming it session, in that case.

@ryzokuken

Copy link
Copy Markdown
ContributorAuthor

This is a single documentation change, and I believe it should be fast-forwarded. Please respond with 👍 to approve fast-forwarding this.

@vsemozhetbytvsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 21, 2018
@vsemozhetbyt

Copy link
Copy Markdown
Contributor

@vsemozhetbytvsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 21, 2018
@ryzokuken

Copy link
Copy Markdown
ContributorAuthor

CI passed! 🎉
Landing it tonight.

Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.
Refs: nodejs/help#877 (comment)
@ryzokuken

Copy link
Copy Markdown
ContributorAuthor

Squashing.

@ryzokuken

Copy link
Copy Markdown
ContributorAuthor

Re-running CI Lite.

CI Lite: https://ci.nodejs.org/job/node-test-pull-request-lite/568/

@ryzokuken

Copy link
Copy Markdown
ContributorAuthor

Passed again, landing.

@ryzokuken

Copy link
Copy Markdown
ContributorAuthor

Landed in 31cbec7

ryzokuken added a commit that referenced this pull request Apr 21, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.
Refs: nodejs/help#877 (comment)
PR-URL: #20193
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.
Refs: nodejs/help#877 (comment)
PR-URL: #20193
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.
Refs: nodejs/help#877 (comment)
PR-URL: nodejs#20193
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Sep 11, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.
Refs: nodejs/help#877 (comment)
PR-URL: nodejs#20193
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Sep 19, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.
Refs: nodejs/help#877 (comment)
PR-URL: nodejs#20193
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.
Refs: nodejs/help#877 (comment)
PR-URL: nodejs#20193
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Add parameters for the callback for the Http2Session:connect event
inline with the pattern in the rest of the documentation.
Refs: nodejs/help#877 (comment)
Backport-PR-URL: #22850
PR-URL: #20193
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Oct 30, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.docIssues and PRs related to the documentations.fast-trackPRs that do not need to wait for 48 hours to land.http2Issues or PRs related to the http2 subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@ryzokuken@Trott@apapirovski@vsemozhetbyt@mcollina@TimothyGu@nodejs-github-bot