-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Conversation
Improve documentation of callback signature of http2Stream.pushStream() function to align with the changes made in nodejs#17406. Fixes: nodejs#18198 Refs: nodejs#17406
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). |
@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? |
Hmm. I thought this would be updated automatically. Maybe you can try to click on this "?" sign here and re-enter the email? |
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. |
There was a problem hiding this comment.
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.
@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. |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
@nephross That the second commit has the proper info now, I think this suffices for proper PR attribution. Thank you! |
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. |
Landed in b886c42 Thank you! |
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