-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
### ERR_STREAM_READ_NOT_IMPLEMENTED | ||
|
||
An attempt was made to use a readable stream that did not implement | ||
[`readable._read()`][]. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks!
103568f
to
d65b9b9
Compare
Should be applicable to v10.x, might be partially applicable to earlier versions. |
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. |
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 :) |
To clarify, my thoughts on the next steps would be:
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 What do you think? @ChALkeR |
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. |
@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 |
@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? |
@joyeecheung I disagree:
|
Labelling as |
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. |
I'm persuadable as well but currently +0.5 on removing. |
Labeling as |
Merge conflict resolved. |
d7f1e4b
to
492ac41
Compare
492ac41
to
f753266
Compare
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>
f753266
to
214844e
Compare
Landed in 214844e. |
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>
This removes two unused error codes.
Those two are:
ERR_STREAM_READ_NOT_IMPLEMENTED
, removed in c979488 (PR stream: make virtual methods errors consistent #18813).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):
ERR_INVALID_ARRAY_LENGTH
ERR_INVALID_DOMAIN_NAME
ERR_STRING_TOO_LARGE
(src: rename ERR_STRING_TOO_LARGE to ERR_STRING_TOO_LONG #19864, old name never ended up in a release)ERR_FS_WATCHER_ALREADY_STARTED
andERR_FS_WATCHER_NOT_STARTED
ERR_ZLIB_BINDING_CLOSED
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
ERR_OUTOFMEMORY
ERR_HTTP2_STREAM_CLOSED
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.
ERR_INVALID_REPL_HISTORY
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), orvcbuild test
(Windows) passes