Skip to content

repl: Adding code for handling multiline history#22153

Closed
antsmartian wants to merge 1 commit intonodejs:masterfrom
antsmartian:multiline-history
Closed

repl: Adding code for handling multiline history#22153
antsmartian wants to merge 1 commit intonodejs:masterfrom
antsmartian:multiline-history

Conversation

@antsmartian
Copy link
Contributor

@antsmartian antsmartian commented Aug 6, 2018 •
edited
Loading

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 few more cases and write a unit test case. But thought, what others think over this? May be there is also a better way to do it.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. labels Aug 6, 2018
@antsmartian
Copy link
Contributor Author

antsmartian commented Aug 6, 2018

Ok looks interesting, test on my mac works perfectly fine. Not sure why CI build is failing though 😕

@maclover7
Copy link
Contributor

maclover7 commented Aug 10, 2018

cc @nodejs/repl

@BridgeAR
Copy link
Member

BridgeAR commented Sep 5, 2018

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.

@antsmartian
Copy link
Contributor Author

antsmartian commented Sep 6, 2018

@BridgeAR Thanks for taking a look at it (Yes, I'm interested in repl too 😃 ). Lets wait for @jdalton comments. Ofcourse, this will be in progress as I have to add few more code and as well as test cases for the same.

jdalton
jdalton reviewed Sep 6, 2018
View reviewed changes
@antsmartian antsmartian force-pushed the multiline-history branch 2 times, most recently from 8093905 to 80084b4 Compare September 9, 2018 04:25
jdalton
jdalton reviewed Sep 26, 2018
View reviewed changes
lib/readline.js Outdated
Copy link
Member

@jdalton jdalton Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ is that debug?

Copy link
Contributor Author

@antsmartian antsmartian Sep 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, yes did for debugging. Will remove them and update the PR with test cases :)

@antsmartian
Copy link
Contributor Author

antsmartian commented Sep 29, 2018

@jdalton Have removed the console statement also added the test. Can you please have a look at it?

@antsmartian
Copy link
Contributor Author

antsmartian commented Oct 4, 2018

@jdalton Can you kindly loop in other folks who also needs to approve this PR before it will get merged? Thanks!

@jdalton
Copy link
Member

jdalton commented Oct 4, 2018

Friendly re-ping of @nodejs/repl 🔊

jasnell
jasnell approved these changes Oct 6, 2018
View reviewed changes
test/parallel/test-repl-persistent-history.js Outdated
Copy link
Member

@jasnell jasnell Oct 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this really shouldn't need the exclusion... just go ahead and wrap.

Copy link
Contributor Author

@antsmartian antsmartian Oct 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell Sure, will take care and push the change. Thanks!

@antsmartian
Copy link
Contributor Author

antsmartian commented Oct 11, 2018 •
edited
Loading

@jasnell : Addressed your review comments.

@antsmartian
Copy link
Contributor Author

antsmartian commented Oct 14, 2018

@jasnell / @jdalton Anything needed from my side? Or else can we merge them? Let me know.

@jasnell
Copy link
Member

jasnell commented Oct 14, 2018

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

@lundibundi
Copy link
Member

lundibundi commented Oct 26, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/18169/

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 26, 2018
@lundibundi
Copy link
Member

lundibundi commented Oct 26, 2018

Friendly ping @nodejs/repl if anyone wants to take another look as quite some time has passed.

@antsmartian
Copy link
Contributor Author

antsmartian commented Nov 1, 2018

@jasnell / @jdalton : Friendly ping 🔔

jdalton
jdalton reviewed Nov 1, 2018
View reviewed changes
@jdalton
Copy link
Member

jdalton commented Nov 2, 2018

This looks more like a bug fix to me.

Trott pushed a commit to Trott/io.js that referenced this pull request Nov 2, 2018
@antsmartian @Trott
lib: repl multiline history support
dd7a3d2 
PR-URL: nodejs#22153
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 2, 2018

Landed in dd7a3d2

@Trott Trott closed this Nov 2, 2018
@antsmartian
Copy link
Contributor Author

antsmartian commented Nov 3, 2018

Thanks everyone!

@antsmartian antsmartian deleted the multiline-history branch November 3, 2018 10:25
@antsmartian antsmartian restored the multiline-history branch November 3, 2018 10:25
@antsmartian antsmartian deleted the multiline-history branch November 3, 2018 10:26
@antsmartian antsmartian restored the multiline-history branch November 3, 2018 10:26
targos pushed a commit that referenced this pull request Nov 3, 2018
@antsmartian @targos
lib: repl multiline history support
b3f3ebf 
PR-URL: #22153
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@antsmartian antsmartian mentioned this pull request Nov 4, 2018
Closed
@antsmartian
Copy link
Contributor Author

antsmartian commented Nov 5, 2018

@targos I think this can be backported to 10.x. What do you think? I would be happy to raise a PR for the same.

@vsemozhetbyt vsemozhetbyt mentioned this pull request Nov 7, 2018
Closed
@BridgeAR BridgeAR mentioned this pull request Nov 14, 2018
Merged
This was referenced Nov 15, 2018
Closed
Closed
@vsemozhetbyt vsemozhetbyt mentioned this pull request Nov 15, 2018
Closed
This was referenced Nov 15, 2018
Closed
Closed
Closed
Closed
@antsmartian antsmartian mentioned this pull request Nov 20, 2018
Closed
4 tasks
@targos
Copy link
Member

targos commented Nov 20, 2018

If this is backported to v10.x, it should be done with #24389

@antsmartian
Copy link
Contributor Author

antsmartian commented Nov 20, 2018

I guess we are good here, I couldn't see the code in 10.x stage : https://github.com/nodejs/node/blob/v0.10-staging/lib/readline.js

@MylesBorins MylesBorins added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v8.x labels Nov 27, 2018
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 27, 2018

This seems like it should perhaps have been Semver-Minor... is that a correct assumption?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@jdalton jdalton jdalton approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.