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.

Sign up for GitHub

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

Jump to bottom

doc: fix http2stream.pushStream error doc #21487

Merged
merged 1 commit into from
Jul 10, 2018
Merged

doc: fix http2stream.pushStream error doc #21487

merged 1 commit into from
Jul 10, 2018

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jun 23, 2018 •
edited
Loading

The old error code ERR_HTTP2_STREAM_CLOSED was removed in commit 0babd18 (pull request #17406), and the testcase for http2stream.pushStream was changed accordingly, but the documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by #21470, which includes some tests for error codes usage and documentation and enforces a stricter format.

Refs: #21470, #17406

Tests are not included — #21470 does that.

/cc @jasnell @addaleax

Checklist
  • 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

@ChALkeR ChALkeR added doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. http2 Issues or PRs related to the http2 subsystem. labels Jun 23, 2018
@nodejs-github-bot
Copy link
Collaborator

@ChALkeR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/134/pipeline

@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 Jun 23, 2018
@ChALkeR ChALkeR requested a review from jasnell June 23, 2018 11:56
@ChALkeR ChALkeR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 23, 2018
@ChALkeR ChALkeR mentioned this pull request Jun 23, 2018
Closed
3 tasks
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 23, 2018 •
edited
Loading

This should land cleanly on 8.x, 9.x and 10.x. Not sure if landing on 9.x makes any sense though.

@ChALkeR ChALkeR added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 24, 2018
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 24, 2018

👍 on this comment for fast-track.

@ChALkeR
doc: fix http2stream.pushStream error doc
094346c 
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request nodejs#17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by nodejs#21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: nodejs#21470
Refs: nodejs#17406
PR-URL: nodejs#21487
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ChALkeR ChALkeR force-pushed the doc-errcodes-remove-broken-ref branch from b8bf9a7 to 094346c Compare July 10, 2018 16:39
@ChALkeR
Copy link
Member Author

ChALkeR commented Jul 10, 2018

Landed in 094346c.

@ChALkeR ChALkeR merged commit 094346c into nodejs:master Jul 10, 2018
targos pushed a commit that referenced this pull request Jul 12, 2018
@ChALkeR @targos
doc: fix http2stream.pushStream error doc
51db88b 
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request #17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by #21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: #21470
Refs: #17406
PR-URL: #21487
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos mentioned this pull request Jul 17, 2018
Merged
This was referenced Jul 18, 2018
Closed
Closed
Closed
Closed
Closed
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
@ChALkeR @kjin
doc: fix http2stream.pushStream error doc
21d5171 
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request nodejs#17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by nodejs#21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: nodejs#21470
Refs: nodejs#17406
PR-URL: nodejs#21487
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
ChALkeR added a commit to ChALkeR/io.js that referenced this pull request Sep 8, 2018
@ChALkeR
test: add test for error codes doc and impl
8e41fd7 
This adds several sanity checks for error codes.

It scans:
 * all natives (js sources),
 * doc/api/*.md documentation
 * src/node_errors.h (errors definition from the C++ side).

There is also a whitelist of manually created errors from JS side,
currently consisting of ERR_HTTP2_ERROR and ERR_UNKNOWN_BUILTIN_MODULE.

Alsom all ERR_NAPI_ codes are whitelisted, as those are created directly
on the cpp side, without declaring them first.

The performed checks:

  1. All errors used from JS should be defined in `internal/errors` and
     present in its .codes object. Whitelist (mentioned above) applies.

  2. All errors instantiated from JS without arguments should support
     0-arguments version.

  3. All errors mentioned in doc should defined either in JS, C++, or
     in the whitelist.

  4. All errors mentioned anywhere should be documented.

  5. Documentation of error codes should be sorted, have no repeats,
     and include exactly one entry for every error code mentioned in
     the documentation, formatted as `/\n### (ERR_[A-Z0-9_]+)\n`.

  6. All doc entries for error codes should have appropriate anchors.

There is also a --report flag, which prints all the current issues and
exits without asserting, for manual inspection.

Individual fixes for those issues are landed in separate commits.

Refs: nodejs#21421
Refs: nodejs#21440
Refs: nodejs#21483
Refs: nodejs#21484
Refs: nodejs#21485
Refs: nodejs#21487
PR-URL: nodejs#21470
kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
@ChALkeR @kjin
doc: fix http2stream.pushStream error doc
94c89ae 
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request nodejs#17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by nodejs#21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: nodejs#21470
Refs: nodejs#17406
PR-URL: nodejs#21487
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
@ChALkeR @kjin
doc: fix http2stream.pushStream error doc
00002a6 
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request nodejs#17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by nodejs#21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: nodejs#21470
Refs: nodejs#17406
PR-URL: nodejs#21487
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
@ChALkeR @BethGriggs
doc: fix http2stream.pushStream error doc
2116ace 
The old error code `ERR_HTTP2_STREAM_CLOSED` was removed in commit
0babd18 (pull request #17406), and the
testcase for http2stream.pushStream was changed accordingly, but the
documentation change was overlooked.

This commit fixes it and aligns the documentation with the testcase.

This is a part of the fixes hinted by #21470, which includes some tests
for error codes usage and documentation and enforces a stricter format.

Refs: #21470
Refs: #17406
Backport-PR-URL: #22850
PR-URL: #21487
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Merged
@beevelop beevelop mentioned this pull request Nov 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell Awaiting requested review from jasnell

Assignees
No one assigned
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. errors Issues and PRs related to JavaScript errors originated in Node.js core. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ChALkeR @nodejs-github-bot @lpinca @cjihrig @vsemozhetbyt @trivikr @BethGriggs