Skip to content
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: Add editor mode support #7275

Closed
wants to merge 6 commits into from

Conversation

princejwesley
Copy link
Contributor

@princejwesley princejwesley commented Jun 12, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

repl, doc

Description of change

Enabling paste editor mode support for REPL (inspired by scala REPL).

> node
> .editor
// Entering editor mode (^D to finish, ^C to cancel)
function test() {
  console.log('tested!');
}

test();

// ^D
tested!
undefined
>

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jun 12, 2016
@benjamingr
Copy link
Member

benjamingr commented Jun 12, 2016

I think this change needs to be voted on or at least discussed first - but personally I'm in favor - thanks for the contribution!

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 12, 2016
@Trott
Copy link
Member

Trott commented Jun 12, 2016

Can you add a test for paste mode? Existing repl tests are in test/parallel/test-repl*.

@princejwesley
Copy link
Contributor Author

@Trott Done!

@Trott
Copy link
Member

Trott commented Jun 13, 2016

/cc @Fishrock123

@Fishrock123 Fishrock123 self-assigned this Jun 13, 2016

stream.write = (msg) => found += msg.replace('\r', '');

const expected = `${terminalCode}.paste
Copy link
Contributor

Choose a reason for hiding this comment

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

This code looks strange because of the indentation. Maybe change it to string concatenations.

@Trott
Copy link
Member

Trott commented Jun 14, 2016

The doc could probably use some information about what paste mode is.

@Trott
Copy link
Member

Trott commented Jun 14, 2016

Based on git shortlog -n -s lib/repl.js --since 'one year ago', /cc @thefourtheye @bnoordhuis @addaleax

@Fishrock123
Copy link
Contributor

Based on git shortlog -n -s lib/repl.js --since 'one year ago'

Keep in mind that more of us have touched the internal module.

I probably won't get to taking a look soon, but I'll leave my assignee as a self reminder for now.

@@ -457,7 +468,8 @@ function REPLServer(prompt,

// If error was SyntaxError and not JSON.parse error
if (e) {
if (e instanceof Recoverable && !self.lineParser.shouldFail) {
if (e instanceof Recoverable && !self.lineParser.shouldFail &&
!sawCtrlD) {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably line up with e instanceof Recoverable…

self.turnOffPasteMode = () => {
self.pasteMode = false;
self.setPrompt(self._initialPrompt);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

better as a .prototype method?

@Fishrock123
Copy link
Contributor

Could someone elaborate on the benefits of this vs just normally pasting? Is it because you can edit the paste?

@Fishrock123
Copy link
Contributor

@princejwesley
Copy link
Contributor Author

princejwesley commented Jun 17, 2016

@Fishrock123

  • All lines of code entered in the paste mode will be executed at one shot. No more guesses on whether the emitted error is recoverable or not! It fixes most of the multiline mode issues.
  • Even though its called paste mode (copied the name and idea from scala REPL), Its precisely an instant, line buffered editor (Lazy multiline mode).
  • In paste mode, tab completion, history navigation shortcuts are turned off. All other terminal shortcuts are preserved
  • We can cancel the whole input but the history is preserved since terminal is line buffered(Worked the same way like scala REPL)

@Fishrock123
Copy link
Contributor

@princejwesley Shouldn't tab completion still work?

Sounds like I should give this a try. I'm not so sure .paste is appropriate... maybe .editor?

@princejwesley
Copy link
Contributor Author

@Fishrock123 Yes. .editor is good. I turned off tab complete to keep editor clean from tab completion output! Comment those statements and try!

@princejwesley
Copy link
Contributor Author

@princejwesley
Copy link
Contributor Author

CI is green

@trevnorris
Copy link
Contributor

@princejwesley To confirm, there's nothing here that checks for tab press anymore (i.e. would conflict with #7754)?

@princejwesley
Copy link
Contributor Author

@trevnorris Good point. I'll pull the patch and test locally.

@princejwesley
Copy link
Contributor Author

```js
> node
> .editor
// Entering editor mode (^D to finish, ^C to cancel)
function test() {
  console.log('tested!');
}

test();

// ^D
tested!
undefined
>
```
@princejwesley
Copy link
Contributor Author

@princejwesley
Copy link
Contributor Author

Irrelevant CI failures: #3520, #3626, #3550 @Fishrock123

@jasnell
Copy link
Member

jasnell commented Aug 6, 2016

New run to see if we can get all green... https://ci.nodejs.org/job/node-test-pull-request/3551/

@princejwesley
Copy link
Contributor Author

@jasnell CI is green

@jasnell
Copy link
Member

jasnell commented Aug 6, 2016

Woo! 😀 LGTM, let's land it

princejwesley added a commit that referenced this pull request Aug 6, 2016
```js
> node
> .editor
// Entering editor mode (^D to finish, ^C to cancel)
function test() {
  console.log('tested!');
}

test();

// ^D
tested!
undefined
>
```

PR-URL: #7275
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@princejwesley
Copy link
Contributor Author

Landed in b779eb42

@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
```js
> node
> .editor
// Entering editor mode (^D to finish, ^C to cancel)
function test() {
  console.log('tested!');
}

test();

// ^D
tested!
undefined
>
```

PR-URL: #7275
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
cjihrig added a commit that referenced this pull request Aug 11, 2016
Notable changes:

* build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576
* child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838
* fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942
* repl: The REPL now supports editor mode. (Prince J Wesley) #7275
* util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013

Refs: #8020
PR-URL: #8070
cjihrig added a commit that referenced this pull request Aug 11, 2016
Notable changes:

* build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576
* child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838
* child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) #7696
* fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942
* repl: The REPL now supports editor mode. (Prince J Wesley) #7275
* util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013

Refs: #8020
PR-URL: #8070
cjihrig added a commit that referenced this pull request Aug 15, 2016
Notable changes:

* build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576
* child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838
* child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) #7696
* fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942
* repl: The REPL now supports editor mode. (Prince J Wesley) #7275
* util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013

Refs: #8020
PR-URL: #8070
cjihrig added a commit to cjihrig/node that referenced this pull request Aug 16, 2016
Notable changes:

* build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) nodejs#7983 and nodejs#7576
* child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) nodejs#7811 and nodejs#7838
* child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) nodejs#7696
* fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) nodejs#7942
* repl: The REPL now supports editor mode. (Prince J Wesley) nodejs#7275
* util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) nodejs#8013

Refs: nodejs#8020
PR-URL: nodejs#8070
@nbering
Copy link

nbering commented Aug 16, 2016

Sorry, a bit late to the issue because I just found it in the release notes. As I understand it this feature is strictly about delaying the execution of the command until you're done entering all the lines? I was hoping when I saw the feature name that it would allow me to fix previous lines if I make an error, but it seems once I hit enter that I can't go back.

@PeterAronZentai
Copy link

Yeah, that was my first impression too, maybe because it is called .editor. Probably .paste would create less confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.