repl: Adding code for handling multiline history #22153
Conversation
|
Ok looks interesting, test on my mac works perfectly fine. Not sure why CI build is failing though |
|
cc @nodejs/repl |
|
It would be great to get multi line support in the repl. I guess the approach itself is fine for as a first start. Looping in @jdalton. You seemed to have interest in the repl as well. |
8093905
to
80084b4
Compare
lib/readline.js
Outdated
| @@ -216,6 +217,7 @@ function Interface(input, output, completer, terminal) { | |||
|
|
|||
| this.history = []; | |||
| this.historyIndex = -1; | |||
| console.log('Yes initialized....\n\n\n', this.history); | |||
There was a problem hiding this comment.
My bad, yes did for debugging. Will remove them and update the PR with test cases :)
|
@jdalton Have removed the console statement also added the test. Can you please have a look at it? |
|
@jdalton Can you kindly loop in other folks who also needs to approve this PR before it will get merged? Thanks! |
|
Friendly re-ping of @nodejs/repl |
| { // Tests multiline history | ||
| env: {}, | ||
| test: ['{', '}', UP, CLEAR], | ||
| expected: [prompt, '{', '... ', '}', '{}\n', prompt, `${prompt}{}`, prompt], // eslint-disable-line max-len |
There was a problem hiding this comment.
nit: this really shouldn't need the exclusion... just go ahead and wrap.
There was a problem hiding this comment.
@jasnell Sure, will take care and push the change. Thanks!
|
@jasnell : Addressed your review comments. |
|
There shouldn't be anything else. A lot of us have been tied up this week with a conference. Hopefully this will be landed here soon |
|
Friendly ping @nodejs/repl if anyone wants to take another look as quite some time has passed. |
|
This looks more like a bug fix to me. |
PR-URL: nodejs#22153 Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in dd7a3d2 |
|
Thanks everyone! |
PR-URL: #22153 Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
@targos I think this can be backported to |
|
If this is backported to v10.x, it should be done with #24389 |
|
I guess we are good here, I couldn't see the code in |
|
This seems like it should perhaps have been Semver-Minor... is that a correct assumption? |
I always wanted to handle multiline history separately. For example, if I enter a multiline statement and then navigate between the history(s), its very annoying as it shows each line, but I expect it to show the executed line.
This is work in progress, but yeah its working and all test cases are passing. I need to handle fewmore cases and write a unit test case. But thought, what others think over this? May be there is alsoa better way to do it.Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes