-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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, events: migrate to internal/errors.js #11400
Conversation
- Replaced existing error messages with new error codes - Fix failing tests
Replaced functions to use arrow functions wherever possible
Replaced existing functions with arrow functions wherever possible
@shubheksha Thanks so much for putting this together. Sorry that it is dragging out for so long due to being a semver-major change. Could you rebase and also squash your commits (I think all the changes should be 1 commit, right?). Thanks! |
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.
LGTM
@shubheksha if you rebase I'll go ahead and land assuming the CI passes. |
@shubheksha just wondering if you are going to be 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 |
I'll follow up |
@refack what do you mean by "I'll follow up". Its been a while so I'm wondering what we should do with this one. |
Ping @refack |
Closing due to the long inactivity. @shubheksha if you would like to pursue this further, please feel free to leave a comment or to open a new PR! I am sorry this could not land as is and your contribution is highly appreciated nevertheless. |
Fixes #11273
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
errors, events, test