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: remove 2 unused error codes from errors.md #21491

Merged
merged 1 commit into from
Aug 6, 2018

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Jun 23, 2018

This removes two unused error codes.

Those two are:

  1. ERR_STREAM_READ_NOT_IMPLEMENTED, removed in c979488 (PR stream: make virtual methods errors consistent #18813).
  2. ERR_VALUE_OUT_OF_RANGE, removed in d022cb1 (PR lib: combine similar error codes #17648).

There are concerns about removing error codes voiced by @jasnell and @joyeecheung here: #21470 (comment), implying that the documentation for all removed error codes should stay forever in doc/api/errors.md (hence I requested their review here).

As I said in that PR — we have a history of removing error codes from that document, and those two remainders look more like an accident than a result of an established process to keep error codes.

List of previous removals (might be or not be parital):

  • 186857f — removes ERR_INVALID_ARRAY_LENGTH
  • 564048d — removes ERR_INVALID_DOMAIN_NAME
  • 3626944 — renames ERR_STRING_TOO_LARGE (src: rename ERR_STRING_TOO_LARGE to ERR_STRING_TOO_LONG #19864, old name never ended up in a release)
  • 301f6cc — removes ERR_FS_WATCHER_ALREADY_STARTED and ERR_FS_WATCHER_NOT_STARTED
  • 5e3f516 — removes/renames ERR_ZLIB_BINDING_CLOSED
  • 6e1c25c — removes ERR_HTTP_INVALID_CHAR, ERR_HTTP2_ALREADY_SHUTDOWN, ERR_HTTP2_FRAME_ERROR, ERR_HTTP2_HEADER_REQUIRED, ERR_HTTP2_HEADERS_OBJECT, ERR_HTTP2_INFO_HEADERS_AFTER_RESPOND, ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK, ERR_NAPI_CONS_PROTOTYPE_OBJECT, ERR_PARSE_HISTORY_DATA, ERR_TLS_RENEGOTIATION_FAILED
  • a82b1b7 — removes ERR_OUTOFMEMORY
  • 0babd18 — removes/renames ERR_HTTP2_STREAM_CLOSED
  • 1cdb41f — removes ERR_HTTP2_ERROR, ERR_UNKNOWN_BUILTIN_MODULE
    (ERR_HTTP2_HEADER_SINGLE_VALUE is just moved around)
    Revert in doc: restore documentation for two error codes #21484.
  • e6b69b9 — removes ERR_INVALID_REPL_HISTORY
  • 1b54371 — renames ERR_STREAM_HAS_STRINGDECODER

See e.g. the first one from that list: #20484, which happened recently, is about an explicit error code removal, has been reviewed by @jasnell and @joyeecheung, and removed an error code that was present in released versions.

To my understanding, the current process is to remove obsolete error codes — this is what the list of commits above hints, and else there would have been more than two documented but unused error codes (in fact, there would have been a lot of those).

I propose to align the current documentation with that (i.e. to remove those two unused error codes from the doc), then, if/when we come to a conclusion to change that process and revert the removal — land them all back, perhaps to a separate page or whatever.


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.

Tests are not included — #21470 does that.

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. labels Jun 23, 2018
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Jun 23, 2018
### ERR_STREAM_READ_NOT_IMPLEMENTED

An attempt was made to use a readable stream that did not implement
[`readable._read()`][].
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the bottom reference for this link can also be removed now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

@ChALkeR ChALkeR force-pushed the doc-errcodes-remove-unused branch from 103568f to d65b9b9 Compare June 23, 2018 23:24
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 24, 2018

Should be applicable to v10.x, might be partially applicable to earlier versions.

@ChALkeR ChALkeR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 24, 2018
@joyeecheung
Copy link
Member

joyeecheung commented Jun 24, 2018

Regarding #20484: I think that was an oversight and now taking a look of the whole picture it should not have landed. If we want to tidy up the documentation, we should not remove any more error codes but rather put them under a separate section or add something to their changelog indicating that they will no longer appear in new releases (and in addition we should revert the documentation part of those removals). We can white list those errors in our tests.

In my understanding, the point of checking that all exiting error codes have documentation is to make sure when someone encounter an error they would be able to look up the meaning of that error with something that's easy to search for. It kind of loses the whole point if we remove documentation of error codes that existed.

@joyeecheung
Copy link
Member

Also: thanks for looking into the removal of those error codes! I was aware of the inconsistency of the documentation but have never been able to get to the bottom of that, so I really appreciate that you took the time to figure out the history of those codes :)

@joyeecheung
Copy link
Member

joyeecheung commented Jun 24, 2018

To my understanding, the current process is to remove obsolete error codes — this is what the list of commits above hints, and else there would have been more than two documented but unused error codes (in fact, there would have been a lot of those).

I propose to align the current documentation with that (i.e. to remove those two unused error codes from the doc), then, if/when we come to a conclusion to change that process and revert the removal — land them all back, perhaps to a separate page or whatever.

To clarify, my thoughts on the next steps would be:

  1. The current practice of removing obsolete error codes should be corrected and stopped.
  2. We should document (in doc/guides/using-internal-errors.md) that error codes should not be removed but moved to a separate section in doc/api/errors.md dedicated to obsolete codes
  3. We should revert the removals of error code documentations so that the documentation is back to a state consistent with what 2 proposes
  4. We can enforce the check to prevent removal of error code documentation in the future using tests like test: add test for error codes doc and impl #21470

The reasoning for 1 and 2 is that we should avoid the scenario where someone encounters an error in an existing release but cannot find the documentation of that code in errors.html, which is happening right now but should be stopped.

What do you think? @ChALkeR

@targos
Copy link
Member

targos commented Jun 24, 2018

The reasoning for 1 and 2 is that we should avoid the scenario where someone encounters an error in an existing release but cannot find the documentation of that code in errors.html, which is happening right now but should be stopped.

I'm not convinced this is a valid scenario. The error should still be accessible in the documentation for the release in which it existed, and people should read the documentation corresponding to the release version that they are using. When we remove an API, the corresponding documentation is also removed from master/future releases. I'm not sure errors should be treated differently.

@joyeecheung
Copy link
Member

When we remove an API, the corresponding documentation is also removed from master/future releases. I'm not sure errors should be treated differently.

@targos Correct me if I'm wrong, in my understanding, when we remove an previously documented API the documentation is not completely wiped out but instead it is supposed to appear in deprecations.md?

@targos
Copy link
Member

targos commented Jun 24, 2018

@joyeecheung True. I haven't made up my mind yet, though. What is the benefit of keeping old error codes in current documentation, knowing that old documentation will always be accessible?
Maybe an argument for it: prevents us from reusing a code for a new, different error?

@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 25, 2018

@joyeecheung I disagree:

  1. The current process looks to be «remove unused error codes» — history of such removals and the fact that there are only 2 missed hint that. It's not only one PR of errors: remove ERR_INVALID_ARRAY_LENGTH #20484 (that you called oversight), it's also tls,http,https: replace url.parse() with WHATWG URL parser #20270, stream: improving error handling #18438, lib: new error implementation #18857, errors: remove ERR_OUTOFMEMORY #17877, and more — that looks like an established practice.
  2. I am not aware of the contrary being documented.
  3. The current state of the error codes list is (a bit) inconsistent. This PR follows the de-facto process and restores the consistency.
  4. If we want the process to be changed — good, I am open to that. But let's file a separate issue/PR about it, I don't think it's in scope here. I.e. I don't see a reason to block error codes removal until we come up with a decision that we want them back in some form. From the comments above I already see that the proposal to keep the old error codes is not unquestionable, so that needs a proper discussion, which can take time, and I would like the docs to be in consistent form in the meantime.
  5. I would argue that if would decide (in a separate issue) that we want to keep the list of the old error codes, it should be done separately (i.e. in a separate file or section) to avoid making this document too large. I think that a list of active error codes here could be of more actual value that a mixed list of all error codes, included inactive ones. So, in my opinion the decision to keep the old error codes would be not as simple as «just revert the removals and don't remove anything else», and we should have a proper discussion on that. This is just not the correct issue for that discussion of a process change.

@ChALkeR ChALkeR requested a review from a team June 25, 2018 06:08
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 25, 2018

Labelling as tsc-review as this looks like it might need more eyes — discussion above shows that this is controversial.

@ChALkeR ChALkeR changed the title doc: remove unused error codes from errors.md doc: remove 2 unused error codes from errors.md Jun 25, 2018
@Trott
Copy link
Member

Trott commented Jun 26, 2018

I'm persuadable on this for sure, but my first impulse is to agree with @targos that the documentation for a specific version of Node.js should only contain error codes (and deprecations too!) that are used in that specific version.

I know we want to avoid re-using error codes and deprecation codes but honestly I'm not even sure that's a big deal, especially for error codes. Especially since the codes themselves are descriptive (for errors, not for deprecations), I don't see why re-use would be a problem. It seems like the "don't ever re-use codes" seemed like a good idea when we were devising things before there was real-world use, but turned out (as far as I can tell) to be a YAGNI feature.

That said, if leaving it in the documentation is the most effective way to accomplish something and/or if there is benefit to the end user for having unused codes documented, than I can live with it. I'm about a +0.5 on removing the codes from the docs.

@mhdawson
Copy link
Member

I'm persuadable as well but currently +0.5 on removing.

@ChALkeR ChALkeR added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jun 30, 2018
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 30, 2018

Labeling as tsc-agenda as there seems to be an unresolved disagreement here.

@ChALkeR
Copy link
Member Author

ChALkeR commented Aug 1, 2018

Merge conflict resolved.

@ChALkeR ChALkeR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed tsc-review labels Aug 1, 2018
@targos
Copy link
Member

targos commented Aug 4, 2018

This removes two unused error codes:
 * ERR_STREAM_READ_NOT_IMPLEMENTED, removed in c979488 (PR nodejs#18813).
 * ERR_VALUE_OUT_OF_RANGE, removed in d022cb1 (PR nodejs#17648).

PR-URL: nodejs#21491
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@ChALkeR
Copy link
Member Author

ChALkeR commented Aug 6, 2018

Landed in 214844e.

@ChALkeR ChALkeR merged commit 214844e into nodejs:master Aug 6, 2018
@SirR4T SirR4T mentioned this pull request Aug 6, 2018
27 tasks
targos pushed a commit that referenced this pull request Aug 6, 2018
This removes two unused error codes:
 * ERR_STREAM_READ_NOT_IMPLEMENTED, removed in c979488 (PR #18813).
 * ERR_VALUE_OUT_OF_RANGE, removed in d022cb1 (PR #17648).

PR-URL: #21491
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
vsemozhetbyt pushed a commit that referenced this pull request Aug 26, 2018
PR-URL: #22100
Fixes: #22061
Refs: #21491
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 27, 2018
PR-URL: #22100
Fixes: #22061
Refs: #21491
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
PR-URL: #22100
Fixes: #22061
Refs: #21491
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2018
PR-URL: #22100
Fixes: #22061
Refs: #21491
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants