doc: Make createPushResponse() more detailled. #22366

ghost · 2018-08-17T02:54:22Z

In summary:
We don't know what will return when successful or failure for
the callback of the function. So make it more detailled.

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Trott · 2018-08-17T04:21:27Z

@nodejs/documentation @nodejs/http2

sebdeckers · 2018-08-17T07:19:38Z

doc/api/http2.md

-The callback will be called with an error with code `ERR_HTTP2_INVALID_STREAM`
-if the stream is closed.
+given [`Http2Stream`] on a newly created `Http2ServerResponse` as the callback
+parameter when callback is successful. And for the closed stream, The callback


Grammar/spelling nit:

For the closed stream, the callback

Any spelling wrong or……?

I meant omit the And, and lower-case the.

OK. Thanks. I'll also collect other suggestions and do modifications together :)

mcollina

LGTM

jasnell · 2018-08-17T13:45:22Z

doc/api/http2.md

-Call [`http2stream.pushStream()`][] with the given headers, and wraps the
-given newly created [`Http2Stream`] on `Http2ServerResponse`.
+* `callback` {Function} Callback that is called once
+`http2stream.pushStream()` is finished, or when the stream is closed.


hmm... the when the stream is closed bit is a bit confusing.

The callback is invoked either (a) after the pushed Http2Stream instead has been created and is ready for use or (b) after the attempt to create the pushed Http2Stream has failed or has been rejected.

Well……according to what you suggested, I've included these points in my doc modifications :)

jasnell

Needs additional edits. See comment ^^

vsemozhetbyt · 2018-08-18T08:31:23Z

doc/api/http2.md

-Call [`http2stream.pushStream()`][] with the given headers, and wraps the
-given newly created [`Http2Stream`] on `Http2ServerResponse`.
+* `callback` {Function} Called once `http2stream.pushStream()` is finished,
+or either when the attempt to create the pushed Http2Stream has failed or has


Http2Stream -> `Http2Stream`

We usually align parameter description lines, so this and the next line need 2 space indent.

For 1st: I've only meant to add backticks)

ok, I've found it. Thanks!

vsemozhetbyt · 2018-08-18T08:35:15Z

doc/api/http2.md

+or either when the attempt to create the pushed Http2Stream has failed or has
+been rejected, or the state of `Http2ServerRequest` is closed.
+  * `err` {Error}
+  * `stream` {ServerHttp2Stream} The newly-created `ServerHttp2Stream` object.


Consistency nit: it seems we do not add periods if the description is not a full sentence (see headers description above).

vsemozhetbyt · 2018-08-18T13:02:39Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/587/

jasnell · 2018-08-22T02:47:38Z

doc/api/http2.md

-given newly created [`Http2Stream`] on `Http2ServerResponse`.
+* `callback` {Function} Called once `http2stream.pushStream()` is finished,
+  or either when the attempt to create the pushed `Http2Stream` has failed or
+  has been rejected, or the state of `Http2ServerRequest` is closed


The last bit, , or the state of Http2ServerRequest is closed should be clarified. , or the state of Http2ServerRequest is closed prior to calling the pushStream() method. would be clearer.

OK, it's been fixed.

mcollina · 2018-08-22T07:21:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16670/

mcollina · 2018-08-22T13:21:53Z

@Maledong can you rebase against master? There is the need of a commit there to make CI pass.

ghost · 2018-08-23T00:20:05Z

@mcollina：OK, I'll rebase and have a submit :)

Ref: #22322 In summary: We don't know what will return when successful or failure for the callback of the function. So make it more detailled.

vsemozhetbyt · 2018-08-23T04:19:40Z

CI-lite: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/634/

mcollina · 2018-08-23T07:08:17Z

Landed in e7b4ba9

We don't know what will return when successful or failure for the callback of the function. So this commit makes it more detailled. PR-URL: #22366 Refs: #22322 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

ghost · 2018-08-23T09:13:08Z

Thanks all!

We don't know what will return when successful or failure for the callback of the function. So this commit makes it more detailled. PR-URL: #22366 Refs: #22322 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

We don't know what will return when successful or failure for the callback of the function. So this commit makes it more detailled. PR-URL: nodejs#22366 Refs: nodejs#22322 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

We don't know what will return when successful or failure for the callback of the function. So this commit makes it more detailled. Backport-PR-URL: #22850 PR-URL: #22366 Refs: #22322 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Aug 17, 2018

sebdeckers reviewed Aug 17, 2018

View reviewed changes

mcollina approved these changes Aug 17, 2018

View reviewed changes

jasnell reviewed Aug 17, 2018

View reviewed changes

jasnell requested changes Aug 17, 2018

View reviewed changes

vsemozhetbyt reviewed Aug 18, 2018

View reviewed changes

jasnell reviewed Aug 22, 2018

View reviewed changes

trivikr approved these changes Aug 22, 2018

View reviewed changes

jasnell approved these changes Aug 22, 2018

View reviewed changes

doc: Make createPushResponse() more detailled.

2dc6dce

Ref: #22322 In summary: We don't know what will return when successful or failure for the callback of the function. So make it more detailled.

vsemozhetbyt closed this Aug 23, 2018

vsemozhetbyt reopened this Aug 23, 2018

mcollina closed this Aug 23, 2018

ghost deleted the FixDoc branch August 23, 2018 09:13

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

BethGriggs mentioned this pull request Oct 30, 2018

v8.13.0 proposal #23974

Merged

beevelop mentioned this pull request Nov 20, 2018

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250

Closed

snyk-bot mentioned this pull request Aug 15, 2021

[Snyk] Fix for 1 vulnerabilities ron-a20/node#39

Open

molllyn1 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities molllyn1/node#106

Open

137717unity mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities 137717unity/node#122

Open

kissofire mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities kissofire/node#124

Open

aliceUnhinged613 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities aliceUnhinged613/node#123

Open

ron-a20 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities ron-a20/node#121

Open

wedataintelligence mentioned this pull request Mar 23, 2024

[Snyk] Fix for 2 vulnerabilities wedataintelligence/node#105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: Make createPushResponse() more detailled. #22366

doc: Make createPushResponse() more detailled. #22366

ghost commented Aug 17, 2018

Trott commented Aug 17, 2018

sebdeckers Aug 17, 2018

ghost Aug 17, 2018

sebdeckers Aug 17, 2018

ghost Aug 17, 2018

mcollina left a comment

jasnell Aug 17, 2018

ghost Aug 17, 2018

jasnell left a comment

vsemozhetbyt Aug 18, 2018

ghost Aug 18, 2018 •

edited by ghost

Loading

vsemozhetbyt Aug 18, 2018

ghost Aug 18, 2018

vsemozhetbyt Aug 18, 2018

ghost Aug 18, 2018

vsemozhetbyt commented Aug 18, 2018

jasnell Aug 22, 2018

ghost Aug 22, 2018

mcollina commented Aug 22, 2018

mcollina commented Aug 22, 2018

ghost commented Aug 23, 2018

vsemozhetbyt commented Aug 23, 2018

mcollina commented Aug 23, 2018

ghost commented Aug 23, 2018

doc: Make createPushResponse() more detailled. #22366

doc: Make createPushResponse() more detailled. #22366

Conversation

ghost commented Aug 17, 2018

Trott commented Aug 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Aug 18, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsemozhetbyt commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Aug 22, 2018

mcollina commented Aug 22, 2018

ghost commented Aug 23, 2018

vsemozhetbyt commented Aug 23, 2018

mcollina commented Aug 23, 2018

ghost commented Aug 23, 2018

ghost Aug 18, 2018 •

edited by ghost

Loading