Skip to content

Conversation

@ghost
Copy link

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-botnodejs-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

@nodejs/documentation @nodejs/http2

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

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

LGTM

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

@jasnelljasnell 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 ^^

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

@ghostghostAug 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!

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

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

@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 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@ghost ghost deleted the FixDoc branch August 23, 2018 09:13
@ghost
Copy link
Author

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@targostargos mentioned this pull request Sep 5, 2018
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@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

docIssues and PRs related to the documentations.http2Issues or PRs related to the http2 subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@Trott@vsemozhetbyt@mcollina@sebdeckers@jasnell@trivikr@nodejs-github-bot