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: Make createPushResponse() more detailled. #22366

Closed
wants to merge 1 commit into from
Closed

doc: Make createPushResponse() more detailled. #22366

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 17, 2018

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.

  • 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

@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 Aug 17, 2018
@Trott
Copy link
Member

Trott commented Aug 17, 2018

@nodejs/documentation @nodejs/http2

doc/api/http2.md Outdated
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Grammar/spelling nit:

For the closed stream, the callback

Copy link
Author

Choose a reason for hiding this comment

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

Any spelling wrong or……?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

doc/api/http2.md Outdated
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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Needs additional edits. See comment ^^

doc/api/http2.md Outdated
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
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Http2Stream -> `Http2Stream`
  2. We usually align parameter description lines, so this and the next line need 2 space indent.

Copy link
Author

@ghost ghost Aug 18, 2018

Choose a reason for hiding this comment

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

OK

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

ok, I've found it. Thanks!

doc/api/http2.md Outdated
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

OK

@vsemozhetbyt
Copy link
Contributor

doc/api/http2.md Outdated
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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

OK, it's been fixed.

@mcollina
Copy link
Member

@mcollina
Copy link
Member

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

@ghost
Copy link
Author

ghost commented Aug 23, 2018

@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
Copy link
Contributor

@mcollina
Copy link
Member

Landed in e7b4ba9

@mcollina mcollina closed this Aug 23, 2018
mcollina pushed a commit that referenced this pull request Aug 23, 2018
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 <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@ghost ghost deleted the FixDoc branch August 23, 2018 09:13
@ghost
Copy link
Author

ghost commented Aug 23, 2018

Thanks all!

targos pushed a commit that referenced this pull request Aug 24, 2018
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 <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
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 <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Oct 3, 2018
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 <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
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 <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
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 <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants