-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
repl: Customizable subprompt for multiline input #59566
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
base: main
Are you sure you want to change the base?
repl: Customizable subprompt for multiline input #59566
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #59566 +/- ##
==========================================
- Coverage 88.25% 87.93% -0.33%
==========================================
Files 703 703
Lines 207412 207433 +21
Branches 39893 39707 -186
==========================================
- Hits 183052 182404 -648
- Misses 16313 16987 +674
+ Partials 8047 8042 -5
🚀 New features to boost your workflow:
|
BridgeAR
left a comment
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.
Thanks for opening the PR. I think we should not add anything to readline, but we could add the option.
I also wonder if we just want to set that option to '| ' when we start the internal REPL and not do that for any other REPL instances. That way we would have the default behavior that we want and unbreak external REPLs, while still allowing them to opt-in to the other behavior, when appropriate.
lib/internal/readline/interface.js
Outdated
| * Returns the current multiline prompt. | ||
| * @returns {string} | ||
| */ | ||
| getMultilinePrompt() { |
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 is adding these methods to the readline interface, while we currently only use this in the REPL. I don't think we should regularly expose this API.
Since we already have the symbol anyway, I think we can just use those and directly set the symbol and remove this getter and setter.
lib/internal/repl.js
Outdated
| ignoreUndefined: false, | ||
| useGlobal: true, | ||
| breakEvalOnSigint: true, | ||
| multilinePrompt: opts?.multilinePrompt ?? '| ', |
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 believe this option is currently not used at all? I do think it would in fact be nicer if we used the repl options for defining the value. So we should probably do that?
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.
Hi I'm currently down with fever please give me a day or two. Usually where we discuss and finalize the requirements? I will make the changes soon.
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.
Get well soon! 🫖
Discussing things in the PR is fine.
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.
@pckrishnadas88 I hope you feel better again! Would you mind having another look? :)
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 will try to update it as soon as possible. Will ping you while coding in case I have questions.
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.
@BridgeAR I’ve updated the PR. My branch was behind main by over 200 commits due to a 2–3 week gap, and I got a bit lost on this issue. Could you please review the code and confirm if this aligns with your recommendations? Some tests are still failing, likely because I’m not entirely sure the rebase was completed correctly after encountering conflicts.
Add option to customize the REPL subprompt for multiline input.
Add a option to the REPL to allow custom prompts for multiline input, replacing the default
3bbdf10 to
494e89a
Compare
83c7637 to
de9083b
Compare
Description:
This PR adds the ability to customise the REPL subprompt for multiline input. Previously, the REPL always used the fixed | prompt for multiline statements. With this change, the subprompt can be customised. Once feature finalised doc can be updated.
Feature: Customisable subprompt for multiline input.
Fixes : #59401