errors: minor (SystemError) refactoring #20337
Conversation
nodejs-github-bot
added
the
errors
BridgeAR
added
the
author ready
lib/internal/errors.js
Outdated
| @@ -98,7 +98,6 @@ function sysErrorMessage(prefix, ctx) { | |||
| // and may have .path and .dest. | |||
| class SystemError extends Error { | |||
| constructor(key, context = {}) { | |||
| context = context || {}; | |||
There was a problem hiding this comment.
FWIW, This is not really a duplicated default. This is there to catch the possibility of something like new SystemError('foo', null), which is not handled by the default argument.
There was a problem hiding this comment.
In that case we could still remove the default argument in the parameter list, right?
There was a problem hiding this comment.
The current way SystemErrors are instantiated in the code base warrants that context would at least be a {}, theoretically we don't even need to handle the default argument if we are being careful with new SystemErrors.
|
-1 to fast-tracking. The edge case @jasnell describes likely warrants a test case. |
|
I pushed another commit to remove the default values completely. They were not used at the moment. I also removed spread arguments that were unnecessary and was able to remove the default arguments in It is actually better not to have any default values for I also inlined a function that was only there to generate the message for |
BridgeAR
changed the title
BridgeAR
dismissed
addaleax’s stale review
Removed due to the changed code. PTAL
BridgeAR
removed
the
author ready
BridgeAR
added
the
author ready
BridgeAR
force-pushed
the
remove-obsolete-line
branch
from
3cb2e38 to
67612d0
Compare
|
Rebased due to conflicts. New CI https://ci.nodejs.org/job/node-test-pull-request/14583/ |
This removes the former default values and the spread arguments usage. That was unnecessary and now it does only what is necessary. The `message` function got renamed to `getMessage` to outline that it is actually a function and a helper function was inlined into the SystemError constructor as it was only used there. PR-URL: nodejs#20337 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in 109cfa1 |
This removes the former default values and the spread arguments usage. That was unnecessary and now it does only what is necessary. The `message` function got renamed to `getMessage` to outline that it is actually a function and a helper function was inlined into the SystemError constructor as it was only used there. PR-URL: #20337 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>
|
From: Ruben Bridgewater <notifications@github.com>
Sent: Monday, April 30, 2018 7:56 AM
To: nodejs/node <node@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Subject: Re: [nodejs/node] errors: minor (SystemError) refactoring (#20337)
Closed #20337<#20337>.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<#20337 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AcvRBOPkqCP_vmbRxAxvj8EZn5JKNRWFks5ttyYQgaJpZM4TmPtq>.
|
devsnek
removed
the
author ready
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes