lib: combine similar error codes by starkwang · Pull Request #17648 · nodejs/node

starkwang · 2017-12-13T13:00:11Z

There two similar error codes in lib: ERR_VALUE_OUT_OF_RANGE and ERR_OUT_OF_RANGE. This change is to:

reduce them into ERR_VALUE_OUT_OF_RANGE
make some error messages more detailed

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

Affected core subsystem(s)

cjihrig · 2017-12-13T13:47:59Z

I'm not for or against this, but it definitely defeats the purpose of the error code movement. Theoretically, the idea is that the error message strings can change freely but the codes themselves will never change.

starkwang · 2017-12-13T14:47:23Z

Theoretically, the idea is that the error message strings can change freely but the codes themselves will never change.

@cjihrig Agreed. But I think the reduction of error codes is also important.

TimothyGu

Not too happy about this either, but more "not happy about the fact that we have to do this" than "not happy about the fact that we are doing this".

BridgeAR · 2017-12-13T22:33:04Z

@cjihrig as far as I see it there is still going to be some work about that but it is going to reduce a lot over time.

BridgeAR · 2017-12-13T22:53:13Z

I mark this as ready even though it should probably wait the full 48 hours and how do we handle error codes at the moment? Do we still require two TSC votes on it? I remember that we spoke about loosening that in Vancouver but I am not sure what happened afterwards about it.

jasnell · 2017-12-14T05:58:52Z

changing error codes is definitely server-Major and would require two TSC votes

targos · 2017-12-14T06:04:19Z

I haven't reviewed the code but I'm +1 on the change

maclover7

+1, good to take care of these types of changes while new error codes are still being introduced, hopefully won't be too much ecosystem breakage.

maclover7 · 2017-12-14T15:00:28Z

Should this breaking change be documented somewhere, maybe the ERR_OUT_OF_RANGE should remain in doc/api/errors.md with a note saying it has been replaced, for those still expecting it?

apapirovski

Can we use ERR_OUT_OF_RANGE instead going forward? There are 4 places currently that have ERR_VALUE_OUT_OF_RANGE compared to like 35 uses of the former.

Also, while at it, could you update to use RangeError everywhere instead of TypeError?

Thanks!

starkwang · 2017-12-14T15:45:25Z

Can we use ERR_OUT_OF_RANGE instead going forward? There are 4 places currently that have ERR_VALUE_OUT_OF_RANGE compared to like 35 uses of the former.

@apapirovski Actually there are 5 places using ERR_VALUE_OUT_OF_RANGE and 9 places using ERR_OUT_OF_RANGE in lib

We use ERR_VALUE_OUT_OF_RANGE instead of the other because the error message of ERR_VALUE_OUT_OF_RANGE is more detailed. It gives the expected range and received value in error message. In contrast, the ERR_OUT_OF_RANGE only gives the value name.

Also, while at it, could you update to use RangeError everywhere instead of TypeError?

Yes, I will fix it : )

apapirovski · 2017-12-14T15:47:51Z

Actually there are 5 places using ERR_VALUE_OUT_OF_RANGE and 9 places using ERR_OUT_OF_RANGE in lib

There are 5 total instances of ERR_VALUE_OUT_OF_RANGE, that is 5 total errors that throw that code. There are 37 instances of ERR_OUT_OF_RANGE, 28 in lib/fs.js alone.

We use ERR_VALUE_OUT_OF_RANGE instead of the other because the error message of ERR_VALUE_OUT_OF_RANGE is more detailed. It gives the expected range and received value in error message. In contrast, the ERR_OUT_OF_RANGE only gives the value name.

The error message can be modified on ERR_OUT_OF_RANGE instead. That's the whole point of error codes.

jasnell · 2017-12-14T16:03:43Z

I would rather use the ERR_OUT_OF_RANGE code also.

addaleax · 2017-12-14T16:06:59Z

I think I’d prefer ERR_VALUE_OUT_OF_RANGE, just because it seems not too unlikely that at some point we might want to have something like ERR_INDEX_OUT_OF_RANGE as well? And even if not, the ERR_VALUE_OUT_OF_RANGE doesn’t have that value/index ambiguity in its name.

Edit: Okay, in that case we might also want to call it “out of bounds”, not “out of range”… I still feel like that’s something that might be easy to mix up for non-native speakers of English

apapirovski · 2017-12-14T16:09:17Z

For ERR_INDEX_OUT_OF_RANGE, I would think we would want to just use ERR_OUT_OF_RANGE for both and just have the message say index is out of range or something. I'm not sure I see a benefit to using a unique error code for it.

The benefit of changing ERR_VALUE_OUT_OF_RANGE > ERR_OUT_OF_RANGE is:

less churn
while this remain semver-major, less people are potentially affected by this change.

IMO the introduction of ERR_VALUE_OUT_OF_RANGE was an oversight anyway. We had ERR_OUT_OF_RANGE much longer.

starkwang · 2017-12-14T16:15:23Z

Pushed commit to address comment. Changes are:

use ERR_OUT_OF_RANGE
use RangeError everywhere
document in doc/api/errors.md that ERR_VALUE_OUT_OF_RANGE is longer used and replaced by ERR_OUT_OF_RANGE.

apapirovski · 2017-12-14T16:16:36Z

Also, this could potentially land in two separate commits:

Improve error message for ERR_OUT_OF_RANGE (this can then be backported)
Change ERR_VALUE_OUT_OF_RANGE > ERR_OUT_OF_RANGE (semver-major)

I don't know if it's worth it but something to consider?

apapirovski · 2017-12-14T16:20:04Z

I wouldn't say discarded. Maybe just keep its description and say something like "Superseded by ERR_OUT_OF_RANGE." Maybe this should also somehow mention the Node.js version? I don't know... we don't have a policy around this right now, do we?

Should this go into the deprecations?

Fishrock123 · 2017-12-14T17:35:01Z

I'm not for or against this, but it definitely defeats the purpose of the error code movement. Theoretically, the idea is that the error message strings can change freely but the codes themselves will never change.

I'm not sure about that. The purpose of the movement was to allow the messages to be semver-patch. Actually then changing the code-meaning of the error is still semver-major just as normal.

apapirovski · 2017-12-14T17:36:15Z

Actually then changing the code-meaning of the error

I don't think that's happening in this PR, is it? The meaning is still exactly the same, just with more insight.

starkwang · 2017-12-19T07:07:20Z

Rebased to resolve conflicts and fixed some broken tests.

BridgeAR · 2017-12-21T06:04:08Z

This needs another rebase.

starkwang · 2017-12-21T06:43:49Z

Rebased to resolve conflicts.

BridgeAR · 2017-12-21T06:51:47Z

CI https://ci.nodejs.org/job/node-test-pull-request/12244/

There two similar error codes in lib: "ERR_VALUE_OUT_OF_RANGE" and "ERR_OUT_OF_RANGE". This change is to reduce them into "ERR_VALUE_OUT_OF_RANGE" Fixes: nodejs#17603

starkwang · 2017-12-23T15:58:41Z

Rebased again to resolve conflicts.
Is there anyone can open a new CI and land this PR?

joyeecheung · 2017-12-23T16:14:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12282/

There two similar error codes in lib: "ERR_VALUE_OUT_OF_RANGE" and "ERR_OUT_OF_RANGE". This change is to reduce them into "ERR_VALUE_OUT_OF_RANGE" Fixes: #17603 PR-URL: #17648Fixes: #17603 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

starkwang · 2017-12-24T06:39:07Z

Landed in d022cb1

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>

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>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 13, 2017

maclover7 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 13, 2017

TimothyGu approved these changes Dec 13, 2017
View reviewed changes

jasnell approved these changes Dec 13, 2017
View reviewed changes

BridgeAR approved these changes Dec 13, 2017
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

Trott approved these changes Dec 14, 2017
View reviewed changes

starkwang mentioned this pull request Dec 14, 2017
fs: migrate more errors to internal/errors #17667
Closed
4 tasks

maclover7 approved these changes Dec 14, 2017
View reviewed changes

apapirovski suggested changes Dec 14, 2017
View reviewed changes

addaleax approved these changes Dec 14, 2017
View reviewed changes

starkwang force-pushed the combine-out-of-range-err branch from 4e412cf to 0e23879Compare December 14, 2017 16:07

apapirovski approved these changes Dec 14, 2017
View reviewed changes

starkwang force-pushed the combine-out-of-range-err branch from 0e23879 to aef3c65Compare December 15, 2017 02:10

starkwang force-pushed the combine-out-of-range-err branch from aef3c65 to 94fdcc2Compare December 19, 2017 07:05

starkwang force-pushed the combine-out-of-range-err branch from 94fdcc2 to c0cd8daCompare December 21, 2017 06:43

lib: combine similar error codes
05fe291
There two similar error codes in lib: "ERR_VALUE_OUT_OF_RANGE" and "ERR_OUT_OF_RANGE". This change is to reduce them into "ERR_VALUE_OUT_OF_RANGE" Fixes: nodejs#17603

starkwang force-pushed the combine-out-of-range-err branch from c0cd8da to 05fe291Compare December 23, 2017 15:56

starkwang closed this Dec 24, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017

ChALkeR mentioned this pull request Jun 23, 2018
doc: remove 2 unused error codes from errors.md #21491
Merged
4 tasks

Uh oh!

Uh oh!

Conversation

starkwang commented Dec 13, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

cjihrig commented Dec 13, 2017

Uh oh!

starkwang commented Dec 13, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 13, 2017

Uh oh!

BridgeAR commented Dec 13, 2017

Uh oh!

jasnell commented Dec 14, 2017

Uh oh!

targos commented Dec 14, 2017

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

maclover7 commented Dec 14, 2017

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

starkwang commented Dec 14, 2017

Uh oh!

apapirovski commented Dec 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Dec 14, 2017

Uh oh!

addaleax commented Dec 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Dec 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

starkwang commented Dec 14, 2017

Uh oh!

apapirovski commented Dec 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovskiDec 14, 2017

Choose a reason for hiding this comment

Uh oh!

Fishrock123Dec 14, 2017

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Dec 14, 2017

Uh oh!

apapirovski commented Dec 14, 2017

Uh oh!

starkwang commented Dec 19, 2017

Uh oh!

BridgeAR commented Dec 21, 2017

Uh oh!

starkwang commented Dec 21, 2017

Uh oh!

BridgeAR commented Dec 21, 2017

Uh oh!

starkwang commented Dec 23, 2017

Uh oh!

joyeecheung commented Dec 23, 2017

Uh oh!

starkwang commented Dec 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

starkwang commented Dec 13, 2017•
edited
Loading

apapirovski commented Dec 14, 2017•
edited
Loading

addaleax commented Dec 14, 2017•
edited
Loading

apapirovski commented Dec 14, 2017•
edited
Loading

apapirovski commented Dec 14, 2017•
edited
Loading