-
Notifications
You must be signed in to change notification settings - Fork 30k
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
lib: enforce ASCII order in error code imports #52625
Conversation
Review requested:
|
|
||
[codesSelector](node) { | ||
if (node.value.type !== 'ObjectPattern') { | ||
context.report({ node, message: 'Use destructuring to define error codes used in the file' }); |
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.
What's wrong with importing codes
and then using codes.SOME_ERROR
? (It should have an equally low chance for a conflict I think?)
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.
Correct, that one is just for consistency (as you can see in the change list, destructuring is what happens in most files already)
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 with question on code
RSLGTM on the applied reorders
Landed in 231548b |
PR-URL: #52625 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
PR-URL: nodejs#52625 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
Similarly to a596af0, we can reduce the chances of git conflicts (although much rarer for error codes) and the amount of nitpicking in PRs.