errors: minor (SystemError) refactoring - #20337
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
In that case we could still remove the default argument in the parameter list, right?
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
-1 to fast-tracking. The edge case @jasnell describes likely warrants a test case. |
Sorry, something went wrong.
|
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 message. I renamed that function to getMessage to make clear that it is actually a function and a getter. It is actually better not to have any default values for SystemError because that way we will detect faulty implementations early. I also inlined a function that was only there to generate the message for SystemErrors and I found some dead code in a test. I kept it there because it seems to mirror some internal functionality. |
Sorry, something went wrong.
Removed due to the changed code. PTAL
Sorry, something went wrong.
|
CI https://ci.nodejs.org/job/node-test-pull-request/14540/ |
Sorry, something went wrong.
|
Rebased due to conflicts. New CI https://ci.nodejs.org/job/node-test-pull-request/14583/ |
Sorry, something went wrong.
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>
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>.
|
Sorry, something went wrong.
Checklist