-
Notifications
You must be signed in to change notification settings - Fork 28.8k
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: preprocess only for defaultEval #9752
Conversation
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 pending CI, thank you for doing this :)
}) | ||
}; | ||
|
||
const r = repl.start(options); | ||
r.context = {foo: 'bar'}; | ||
|
||
try { | ||
r.write('foo\n'); | ||
r.write('function f() {}\n'); |
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.
Can I suggest that you add a comment describing why this particular input was chosen?
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.
Sure 😄
// Append a \n so that it will be either | ||
// terminated, or continued onto the next expression if it's an | ||
// unexpected end of input. | ||
return `${cmd}\n`; |
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.
This appending of the \n
isn't needed for custom eval
functions?
Because it was being appended before.
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.
@nunocastromartins Good point. \n
was appended for most of the cases and it was done in same preprocess
. I'll add \n
for all cases as if eval function receives what user entered including the final enter
.
CC: @Fishrock123
Last CI wasn't so happy...? |
New CI: https://ci.nodejs.org/job/node-test-pull-request/4974/ |
@Fishrock123 CI is green 😄 |
@nodejs/collaborators I'll land this tomorrow if there is no objection. |
@addaleax review the changes made after your approval. |
@nodejs/collaborators ping |
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.
Still LGTM
(sorry, I was a bit busy here at NINA)
Code preprocessing is applicable only for default eval function. Therefore, Moved `preprocess` function invocation inside `defaultEval` function. Fixes: nodejs#9743 PR-URL: nodejs#9752 Reviewed-By: Anna Henningsen <[email protected]>
ed4179b
to
b345fab
Compare
Rebased, CI again: https://ci.nodejs.org/job/node-test-pull-request/5157/ |
Code preprocessing is applicable only for default eval function. Therefore, Moved `preprocess` function invocation inside `defaultEval` function. Fixes: #9743 PR-URL: #9752 Reviewed-By: Anna Henningsen <[email protected]>
Landed in 9366b8 |
Code preprocessing is applicable only for default eval function. Therefore, Moved `preprocess` function invocation inside `defaultEval` function. Fixes: #9743 PR-URL: #9752 Reviewed-By: Anna Henningsen <[email protected]>
Code preprocessing is applicable only for default eval function. Therefore, Moved `preprocess` function invocation inside `defaultEval` function. Fixes: nodejs#9743 PR-URL: nodejs#9752 Reviewed-By: Anna Henningsen <[email protected]>
Code preprocessing is applicable only for default eval function. Therefore, Moved `preprocess` function invocation inside `defaultEval` function. Fixes: #9743 PR-URL: #9752 Reviewed-By: Anna Henningsen <[email protected]>
Code preprocessing is applicable only for default eval function. Therefore, Moved `preprocess` function invocation inside `defaultEval` function. Fixes: #9743 PR-URL: #9752 Reviewed-By: Anna Henningsen <[email protected]>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
repl
Description of change
Code preprocessing is applicable only for default
eval function. Therefore, Moved
preprocess
functioninvocation inside
defaultEval
function.Fixes: #9743