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 docs for Http2Session:frameError #20236

Closed
wants to merge 1 commit into from

Conversation

ryzokuken
Copy link
Contributor

@ryzokuken ryzokuken commented Apr 23, 2018

Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @vsemozhetbyt @mcollina @nodejs/http2

@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 Apr 23, 2018
@ryzokuken ryzokuken self-assigned this Apr 23, 2018
@ryzokuken
Copy link
Contributor Author

ryzokuken commented Apr 23, 2018

@ryzokuken
Copy link
Contributor Author

Strange. Rerunning.

@ryzokuken
Copy link
Contributor Author

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

LGTM with nits)

doc/api/http2.md Outdated
@@ -159,18 +159,16 @@ an `Http2Session`.
added: v8.4.0
-->

* `type` {Integer} An integer identifying the frame type.
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 23, 2018

Choose a reason for hiding this comment

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

It seems all three should be {integer} due to type-parser.js.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: You don't need to tell the reader it's an integer if it already has the type {integer}. So instead of everything starting with {integer} An integer identifying the frame type., just say {integer} The frame type.

doc/api/http2.md Outdated
@@ -159,18 +159,16 @@ an `Http2Session`.
added: v8.4.0
-->

* `type` {Integer} An integer identifying the frame type.
* `code` {Integer} An integer identifying the error code.
* `id` {Integer} An integer identifying the stream (or 0 if the frame is not
Copy link
Contributor

Choose a reason for hiding this comment

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

0 -> `0`?

Copy link
Member

@Trott Trott Apr 23, 2018

Choose a reason for hiding this comment

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

{integer} The stream id (or `0` if the frame is not associated with a stream).

doc/api/http2.md Outdated
* `type` {Integer} An integer identifying the frame type.
* `code` {Integer} An integer identifying the error code.
* `id` {Integer} An integer identifying the stream (or 0 if the frame is not
associated with a stream).
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent along with the previous line?

@ryzokuken
Copy link
Contributor Author

Just running LinuxOne: https://ci.nodejs.org/job/node-test-commit-linuxone/643/

@vsemozhetbyt

This comment has been minimized.

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

Please, add 👍 here to approve fast-tracking.

@ryzokuken
Copy link
Contributor Author

@vsemozhetbyt there has to be. It's failing constantly.

Trott
Trott previously requested changes Apr 23, 2018
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Nothing is wrong with LinuxOne. The problem is this PR. It's failing at the doc-building step on the use of {Integer} instead of {integer}.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 23, 2018

Oh, sorry, I've missed this skimming over the log (and I've added this throwing myself recently 🤦‍♂️ ).

@Trott
Copy link
Member

Trott commented Apr 23, 2018

Oh, sorry, I've missed this skimming over the log

Yes, it's very easy to miss. No judgment on anyone. Sorry if it came off that way.

@vsemozhetbyt
Copy link
Contributor

If I get this right, the docs are built in parallel with other tasks and the error message is interleaved with other output in this case. So we need to search for keywords in the page (Error: etc).

@Trott
Copy link
Member

Trott commented Apr 23, 2018

If I get this right, the docs are built in parallel with other tasks and the error message is interleaved with other output in this case. So we need to search for keywords in the page (Error: etc).

Yes, I believe that is correct. Error 1 and Error 2 seem to come up a lot in the relevant error strings so maybe Error (with a space at the end) is worth searching for. And sometimes it even gets buried so early that you have to load the entire log rather than the truncated log that loads by default.

@ryzokuken
Copy link
Contributor Author

@vsemozhetbyt @Trott looks good now?

@ryzokuken
Copy link
Contributor Author

Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)
@trivikr
Copy link
Member

trivikr commented Apr 24, 2018

Looks like lint failed in latest CI https://ci.nodejs.org/job/node-test-linter/18460//console

@ryzokuken
Copy link
Contributor Author

@trivikr it did, now it must be fixed.

@ryzokuken
Copy link
Contributor Author

Rerunning linter again: https://ci.nodejs.org/job/node-test-linter/18462/

@ryzokuken
Copy link
Contributor Author

@Trott care taking a look again and changing your review?

@Trott
Copy link
Member

Trott commented Apr 24, 2018

Lite CI (because changes have been made since the last one and only the linter was re-run): https://ci.nodejs.org/job/node-test-pull-request-lite/579/

@ryzokuken
Copy link
Contributor Author

@Trott It passed!

@Trott Trott dismissed their stale review April 24, 2018 20:48

CI is passing now

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2018
@BridgeAR
Copy link
Member

@Trott @trivikr please always add the author-ready label as described in our guide.

@ryzokuken
Copy link
Contributor Author

CI has passed, all reviews are addressed it has been almost 48 hours (it's fast tracked anyway).

Landing this tonight.

@ryzokuken
Copy link
Contributor Author

Landed in 2a30bfe

@ryzokuken ryzokuken closed this Apr 25, 2018
ryzokuken added a commit that referenced this pull request Apr 25, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20236
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 4, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: #20236
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: nodejs#20236
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
kjin pushed a commit to kjin/node that referenced this pull request Sep 11, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: nodejs#20236
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
kjin pushed a commit to kjin/node that referenced this pull request Sep 19, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: nodejs#20236
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

PR-URL: nodejs#20236
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Improve documentation regarding the callback parameters for the
frameError event for instances of Http2Session, making it inline with
the currently accepted structure, like the rest of the documentation.

Refs: nodejs/help#877 (comment)

Backport-PR-URL: #22850
PR-URL: #20236
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@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
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.

6 participants