-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
errors, querystring: migrate to internal/errors.js #11398
Conversation
- Add documentation for the error code ERR_MALFORMED_URI
- Add new error code - Fix failing tests
Replaced functions with arrow functions where ever possible
@jasnell, I added constructor for |
I am probably bikeshedding here, but I am not sure if
And |
I tend to agree that |
@@ -255,6 +255,13 @@ will affect any stack trace captured *after* the value has been changed. | |||
If set to a non-number value, or set to a negative number, stack traces will | |||
not capture any frames. | |||
|
|||
#### error.code |
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.
Isn't this (and other properties) already added by an older PR?
I don't care one way or the other. |
@jasnell What other error do you suggest? @shubheksha Do you want to rebase this so we can move forward? Thanks! |
@shubheksha sorry it took so long to get this reviewed. Are you able to rebase ? |
@shubheksha just wondering if you are able to rebase this or need any help ? |
I'm closing this because it's been inactive for quite a while. Feel free to reopen or ping a collaborator to get it reopened if needed. |
I'll follow up |
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.
Needs some work in the documentation and the test. Otherwise it seems fine.
qs.stringify({ foo: '\udc00' }); | ||
}, URIError); | ||
}, common.expectsError('ERR_URI_MALFORMED', URIError, 'URI malformed')); |
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.
The expectsError
arguments seem to be faulty. Please compare them to other usages or look them up in the function definition.
Ping @shubheksha are you going to follow up on this? This would require a rebase next to the requested changes. |
Closing this due to long inactivity. @shubheksha I am sorry this could not land as is. If you would like to pursue this further, please leave a comment or open a new PR. |
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
errors, querystring, test