-
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
repl: eager-evaluate input in parens #31943
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.
I don't think there's an immediately obvious behaviour to have here so just aligning the two evaluators is good enough imo.
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 besides the mentioned whitespace issue. It should be handled identically when previewing and actually evaluating the code.
2bba06c
to
8098e49
Compare
8098e49
to
e39244e
Compare
Landed in 20a51b9 |
PR-URL: #31943 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #31943 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
depends on the repl preview changes to land on v12.x |
PR-URL: nodejs#31943 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #31943 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Refs #31820.
This PR updates repl evaluation to eagerly wrap all input in parentheses and adds a regression test using the example given in the above issue. I plan to open a follow-up PR to allow the preview to evaluate the code again unwrapped if the first attempt fails.
cc @BridgeAR @devsnek
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes