lib: repl add .load-editor feature #14503

robbinhan · 2017-07-26T15:14:05Z

support input .load-editor in repl

Fishrock123

Needs docs.

Should be added under https://github.com/nodejs/node/blob/master/doc/api/repl.md#commands-and-special-keys

Fishrock123 · 2017-07-26T15:40:22Z

lib/repl.js

@@ -1267,6 +1267,34 @@ function defineDefaultCommands(repl) {
        '// Entering editor mode (^D to finish, ^C to cancel)\n');
    }
  });
+
+  repl.defineCommand('load-editor', {
+    help: 'Enter load and editor mode',


Maybe 'Load a file and enter editor mode'?

XadillaX · 2017-07-27T02:25:47Z

Would you please update your summary to describe your PR?

eg.

repl: fix .... (not an issue number but describe what exactly you have done)

XadillaX · 2017-07-27T02:28:15Z

test/parallel/test-repl-definecommand.js

 inputStream.write('.help\n');
-assert(/\n\.say1     help for say1\n/.test(output),


any reason to have these modification? (a blank line and some spaces)

because add .load-editor command need more space for fomat

> .help .break Sometimes you get stuck, this gets you out .clear Break, and also clear the local context .editor Enter editor mode .exit Exit the repl .help Print this help message .load Load JS from a file into the REPL session .load-editor Load a file and enter editor mode .save Save all evaluated commands in this REPL session to a file .say1 help for say1 .say2 >

Trott

Thanks for this! It would seem that it needs a test.

XadillaX · 2017-07-27T05:42:44Z

test/parallel/test-repl-definecommand.js

@@ -34,8 +34,9 @@ r.defineCommand('say2', function() {
  this.displayPrompt();
 });

+


How about this blank line?

robbinhan · 2017-07-27T12:18:06Z

@Fishrock123 I updated doc
@Trott I added a test
@XadillaX I removed it

test added, dismissing review

docs added

jasnell · 2017-08-08T23:34:49Z

doc/api/repl.md

@@ -39,6 +39,7 @@ The following special commands are supported by all REPL instances:
 * `.load` - Load a file into the current REPL session.
  `> .load ./file/to/load.js`
 * `.editor` - Enter editor mode (`<ctrl>-D` to finish, `<ctrl>-C` to cancel)
+* `.load-editor` - Load a file and enter editor mode (`<ctrl>-D` to finish, `<ctrl>-C` to cancel)


It would be helpful to have an example

Do you mean add an example in doc?

Yeah. For instance, I presume there's a way of providing the file name to load but that's not captured here

Just how .load does it should be fine, the second line.

Thanks, I have updated doc file

jasnell · 2017-08-08T23:35:58Z

test/parallel/test-repl-load-editor.js

+  const stream = new common.ArrayStream();
+  const outputStream = new common.ArrayStream();
+
+  stream.write = () => { throw new Error('Writing not allowed!'); };


stream.write = common.mustNotCall('writing not allowed');

thank you for your help

Fishrock123

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

lance · 2017-08-17T13:53:31Z

In #14861 (comment) I discovered that .load will only work with some .js files, but not all depending on the formatting. I wonder if it makes more sense to just have .load capable of loading and executing any JS file, vs having it work for some but not all. I'm not sure the benefit of adding additional surface area to the REPLServer interface with .load-editor when .load itself should be improved (imo).

robbinhan · 2017-08-18T09:00:30Z

@lance
In my opinion, these are two different command. ‘.load’ just a command to lad file, and does’t do more things rather than that. ‘.load-editor’ means that load file and change into editor mode.

lance · 2017-08-18T11:10:17Z

@robbinhan I see your point. But it seems to me that, as-is, .load is broken and should be fixed. A reasonable fix for it would be to enable editor mode when .load is executed. In doing that, there is no need for .load-editor.

As I said in my PR, I didn't mean to encroach on what you were doing here, I was just trying to address #14022. I wasn't aware of your PR until a comment from @Fishrock123. I definitely appreciate your contribution here, and I am willing to go with whatever approach makes the most sense. In this case, I feel it's fixing .load, but I will defer to others' opinions.

robbinhan · 2017-08-19T12:32:48Z

@lance So, does improve .load command to enable editor mode, than remove .load-editor command is the best choice？

lance · 2017-08-19T14:12:26Z

@robbinhan in my opinion, yes. But I want to hear from other @nodejs/collaborators on it.

jasnell · 2017-08-24T04:53:12Z

I'm thinking that the other PR is likely the better option. Not quite ready to sign off on it tho.

BridgeAR · 2017-08-25T04:39:03Z

I looked at both of these and I am definitely in favor of #14861. As @lance already pointed out it is more like a bug fix instead of adding a new feature. .load is simply not working as it is intended to and #14861 fixes that. Because of those reasons I am actually opposed to landing this.

@robbinhan I appreciate your work a lot nevertheless!

BridgeAR · 2017-08-28T00:07:19Z

Closing due to the already mentioned reasons.

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 26, 2017

Fishrock123 previously requested changes Jul 26, 2017

View reviewed changes

robbinhan force-pushed the master branch 2 times, most recently from ab13971 to b217b78 Compare July 27, 2017 02:02

XadillaX reviewed Jul 27, 2017

View reviewed changes

robbinhan changed the title ~~lib: repl for #14022~~ lib: repl add .load-editor feature Jul 27, 2017

Trott previously requested changes Jul 27, 2017

View reviewed changes

XadillaX reviewed Jul 27, 2017

View reviewed changes

robbinhan force-pushed the master branch 2 times, most recently from 97dc9c9 to 6137b22 Compare July 27, 2017 10:18

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 30, 2017

jasnell reviewed Aug 8, 2017

View reviewed changes

robbinhan force-pushed the master branch from 6137b22 to 90fef75 Compare August 9, 2017 03:56

robbinhan added 4 commits August 15, 2017 13:23

lib: repl for nodejs#14022

cd1aa37

lib: repl for nodejs#14022

141b682

lib: repl for nodejs#14022

22b2c38

lib: repl for nodejs#14022

10b36b3

robbinhan force-pushed the master branch from 90fef75 to 10b36b3 Compare August 15, 2017 05:24

jasnell approved these changes Aug 16, 2017

View reviewed changes

Fishrock123 approved these changes Aug 16, 2017

View reviewed changes

Fishrock123 mentioned this pull request Aug 16, 2017

repl: allow multiline arrow functions in .load #14861

Closed

3 tasks

BridgeAR closed this Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: repl add .load-editor feature #14503

lib: repl add .load-editor feature #14503

robbinhan commented Jul 26, 2017

Fishrock123 left a comment

Fishrock123 Jul 26, 2017

XadillaX commented Jul 27, 2017 •

edited

Loading

XadillaX Jul 27, 2017

robbinhan Jul 27, 2017

XadillaX Jul 27, 2017

Trott left a comment

XadillaX Jul 27, 2017

robbinhan commented Jul 27, 2017

jasnell Aug 8, 2017

robbinhan Aug 9, 2017

jasnell Aug 9, 2017

Fishrock123 Aug 14, 2017

robbinhan Aug 15, 2017

jasnell Aug 8, 2017

robbinhan Aug 9, 2017

Fishrock123 left a comment

lance commented Aug 17, 2017 •

edited

Loading

robbinhan commented Aug 18, 2017

lance commented Aug 18, 2017

robbinhan commented Aug 19, 2017

lance commented Aug 19, 2017

jasnell commented Aug 24, 2017

BridgeAR commented Aug 25, 2017

BridgeAR commented Aug 28, 2017

		inputStream.write('.help\n');
		assert(/\n\.say1 help for say1\n/.test(output),

		@@ -34,8 +34,9 @@ r.defineCommand('say2', function() {
		this.displayPrompt();
		});

lib: repl add .load-editor feature #14503

lib: repl add .load-editor feature #14503

Conversation

robbinhan commented Jul 26, 2017

Fishrock123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XadillaX commented Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robbinhan commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 left a comment

Choose a reason for hiding this comment

lance commented Aug 17, 2017 • edited Loading

robbinhan commented Aug 18, 2017

lance commented Aug 18, 2017

robbinhan commented Aug 19, 2017

lance commented Aug 19, 2017

jasnell commented Aug 24, 2017

BridgeAR commented Aug 25, 2017

BridgeAR commented Aug 28, 2017

XadillaX commented Jul 27, 2017 •

edited

Loading

lance commented Aug 17, 2017 •

edited

Loading