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: improve callback documentation of http2Stream.pushstream() #18258

Closed
wants to merge 3 commits into from

Conversation

nephross
Copy link
Contributor

Improve documentation of callback signature of
http2Stream.pushStream() function to align with
the changes made in #17406.

Fixes: #18198
Refs: #17406

Checklist
Affected core subsystem(s)

doc

Improve documentation of callback signature of
http2Stream.pushStream() function to align with
the changes made in nodejs#17406.

Fixes: nodejs#18198
Refs: nodejs#17406
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Jan 19, 2018
@vsemozhetbyt
Copy link
Contributor

Hi, @nephross. Welcome to the Node.js project and thank you for your contribution!

It seems your local git email settings is out of sync with the GitHub email settings, so GitHub does not associate your commit with your account.

To fix this, see "Setting up your local environment" (the last paragraph in particular).

@nephross
Copy link
Contributor Author

@vsemozhetbyt Thank you for pointing it out, found the error and fixed for my local repo, but how do I fix it for the commit already pushed?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Jan 19, 2018

Hmm. I thought this would be updated automatically. Maybe you can try to click on this "?" sign here and re-enter the email?

1

doc/api/http2.md Outdated
@@ -1197,8 +1197,10 @@ added: v8.4.0
**Default:** `false`
* `parent` {number} Specifies the numeric identifier of a stream the newly
created stream is dependent on.
* `callback` {Function} Callback that is called once the push stream has been
initiated.
* `callback` {Function} Callback that is called once the push stream has been initiated.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: please wrap lines at 80 chars.

@nephross
Copy link
Contributor Author

@vsemozhetbyt I don't see the ? unfortunately. I have tried amending the commit author but it is not reflecting upstream.

doc/api/http2.md Outdated
* `callback` {Function} Callback that is called once the push stream has been
initiated.
* `callback` {Function} Callback that is called once the push stream has been
initiated.
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: I wonder if the hard line break is rendered here due to missing indentation.

doc/api/http2.md Outdated
initiated.
* `err` {Error}
* `pushStream` {[`ServerHttp2Stream`][]} The returned pushStream object.
* `headers` {[Headers Object][]} Headers object the pushStream was initiated with.
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: this line also needs wrapping (currently 84 characters).

Sorry for the pettiness) This can be fixed by a commit lander if it is not convenient for you to amend the commit or push more commits)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's all good. My first pr, so I'm here to learn.

@vsemozhetbyt
Copy link
Contributor

@nephross That the second commit has the proper info now, I think this suffices for proper PR attribution. Thank you!

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 19, 2018
@vsemozhetbyt
Copy link
Contributor

Memo for the lander: I am not sure how will squashing go with the different commit authors and how this will affect the promotion to Contributors, so we may need to be a bit more careful here.

@vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 21, 2018
vsemozhetbyt pushed a commit that referenced this pull request Jan 24, 2018
Improve documentation of callback signature of
http2Stream.pushStream() function to align with
the changes made in #17406.

PR-URL: #18258
Fixes: #18198
Refs: #17406
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@vsemozhetbyt
Copy link
Contributor

Landed in b886c42

Thank you!

evanlucas pushed a commit that referenced this pull request Jan 30, 2018
Improve documentation of callback signature of
http2Stream.pushStream() function to align with
the changes made in #17406.

PR-URL: #18258
Fixes: #18198
Refs: #17406
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong v9.4.0 documentation for ServerHttp2Stream#pushStream()
7 participants