-
Notifications
You must be signed in to change notification settings - Fork 30.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
repl: named anonymous functions in repl.js #21765
Conversation
Thank you @babamihai for your first PR in Node.js core! 🎉 |
@@ -1401,7 +1401,7 @@ function _turnOffEditorMode(repl) { | |||
function defineDefaultCommands(repl) { | |||
repl.defineCommand('break', { | |||
help: 'Sometimes you get stuck, this gets you out', | |||
action: function() { |
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 name corresponds to the property name here so this is not really anonymous. The new name is more accurate though. Maybe use defineCommandBreakAction
for consistency?
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 linter enforces the same name. So all these functions can only be named action
.
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.
In that case, it might make sense to use the shorthand syntax:
{
action() {}
}
@@ -1466,7 +1466,7 @@ function defineDefaultCommands(repl) { | |||
|
|||
repl.defineCommand('load', { | |||
help: 'Load JS from a file into the REPL session', | |||
action: function(file) { | |||
action: function defineDefineCommandLoadAction(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.
defineCommandLoadAction
?
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 the comments addressed.
@babamihai Can you please address the comments and update the PR as documented here |
@babamihai Thank you for the PR! Closing due to inactivity. If you'd like to come back this, just reopen or leave a comment here. Please feel free to ask for any help you need in getting this over the finish line. |
refs: #8913
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes