repl: add hint as to how to exit #20617

monkingxue · 2018-05-09T02:50:03Z

add friendly tips about how to exit repl.

Fixes: #19021

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mscdex · 2018-05-09T03:06:10Z

I think it would be better to at least check that exit and/or quit (whichever was entered) are not already defined before making the suggestion.

mscdex · 2018-05-09T03:07:46Z

Also, can you remove the merge commit?

monkingxue · 2018-05-09T03:40:24Z

@mscdex Ok, I will remove the commit. And this is my first commit, so sorry.

monkingxue · 2018-05-09T03:44:33Z

This is the effect changed now.

$ ./out/Release/node
> exit
(To exit, press ^D or type .exit)
> quit
(To exit, press ^D or type .exit)
> exit = 1
1
> exit
1
> quit = 2
2
> quit
2

monkingxue · 2018-05-09T04:11:20Z

When exit and/or quit have been defined, the runInThisContext function will not throw an error, but our prompt just be output in the catch block.

Trott · 2018-05-09T05:44:05Z

@nodejs/repl

Trott · 2018-05-09T05:44:58Z

Also pinging @princejwesley (who should totally let us know if they want to be added to @nodejs/repl).

vsemozhetbyt · 2018-05-09T10:52:54Z

Is this semver-major? Can there be cases when changed output is breaking?

vsemozhetbyt · 2018-05-09T10:53:59Z

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

princejwesley · 2018-05-09T11:31:32Z

Its good to be generic for all defined commands. Its like hint message property while defining commands and emit the message to help the user only when the command is not defined in the context (global or context) and there is no buffered command.

const context = this.useGlobal ? global : this.context;
if (context[cmd] === undefined && this.commands[cmd] && this[kBufferedCommandSymbol] === '') {
    // emit this.commands[cmd].hint
}

addaleax · 2018-05-09T17:24:59Z

lib/repl.js

    var wrappedCmd = false;
    var awaitPromise = false;
    var input = code;

+    if ((trimCmd = code.trim()) && (trimCmd === 'exit' || trimCmd === 'quit')) {


Just a tiny style thing, but could you maybe turn trimCmd = code.trim() into its own line? Also, don’t be shy to use a verbose name like trimCommand/isExitCommand.

Thank you soooo much about your advice! I will modify the code as soon.

addaleax · 2018-05-09T17:25:53Z

lib/repl.js

+        if (isExitCmd) {
+          self.outputStream.write('(To exit, press ^D or type .exit)\n');
+          return self.displayPrompt();
+        } else {


I think you don’t really need to add the else { … } here since you return early in the if() {} block anyway.

monkingxue · 2018-05-10T02:44:12Z

@princejwesley The command must start with ., like .help, .exit and so on. So we cannot define a variable named same as command.

monkingxue · 2018-05-10T02:55:25Z

Hi, this is my first time offering pr to nodejs, so what should I do after commit the changes? Thank you so much~

jasnell · 2018-05-10T14:10:45Z

@monkingxue ... the process is: a number of us will review the commits and offer feedback as necessary. Once we're relatively sure it's ready, someone with commit access to kick off a CI test run in jenkins at ci.nodejs.org, and if it looks good we'll get it landed. So far you've done everything you need to do :-)

monkingxue · 2018-05-10T15:23:39Z

@jasnell Thank u sooo much~ It is an honor to pr for nodejs !

princejwesley · 2018-05-10T15:27:29Z

@monkingxue Internally commands are not prefixed with . 😄 .

I don't think, its a good idea to check in the catch block after executing the code. because Its easy to define exit in such a way that it will throw error.

e.g.

$ node
> Object.defineProperty(global, 'exit', { get: function() { throw new Error('catch me') } });
Object [global] {
...
}
> exit
Error: catch me
    at get (repl:1:65)
>

I suggest to try something like this.

   const context = this.useGlobal ? global : this.context;
   if (isExitCommand  && !context.hasOwnProperty(trimCommand)) {
      // emit hint
   }

@nodejs/repl what do you say?

monkingxue · 2018-05-10T15:51:41Z

@princejwesley oh I see! You're right, i will fix the code, thank u suggestion！

princejwesley · 2018-05-10T18:44:58Z

lib/repl.js

@@ -338,7 +338,8 @@ function REPLServer(prompt,
          }
        }
      } catch (e) {
-        if (isExitCommand) {
+        const context = this.useGlobal ? global : this.context;
+        if (isExitCommand && !context.hasOwnProperty(trimCommand)) {


this can be done before executing the code, right?

You mean define the context out of the catch block ?

No. the whole test condition. check this before executing vm.runInContext

ok I got it and I have modified the code, thank u~

BridgeAR

LGTM with my comments addressed.

BridgeAR · 2018-05-18T01:39:12Z

lib/repl.js

    var wrappedCmd = false;
    var awaitPromise = false;
    var input = code;
+    var trimCommand = code.trim();


Would you be so kind and rename the variable to trimmedCommand? Otherwise I would confuse it with a function :-)

BridgeAR · 2018-05-18T01:41:15Z

lib/repl.js

        if (err && err.code === 'ERR_SCRIPT_EXECUTION_INTERRUPTED') {
-          // The stack trace for this case is not very useful anyway.


Nit: this is a unrelated change and I think the comment is better the way it was before.

BridgeAR · 2018-05-18T03:30:52Z

lib/repl.js

    var wrappedCmd = false;
    var awaitPromise = false;
    var input = code;
+    var trimCommand = code.trim();
+
+    if ((trimCommand === 'exit' || trimCommand === 'quit')) {


Please remove the extra brackets.

Imitate python repl, when the user enters 'exit' or 'quit', no longer prompt 'Reference Error', but prompts 'To exit, press ^D or type .exit'. If the user defines variables named 'exit' or 'quit' , only the value of the variables are output Fixes: nodejs#19021

Before output the hint, check if defined the command-named variable as an error

BridgeAR · 2018-05-18T10:19:48Z

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

Imitate python repl, when the user enters 'exit' or 'quit', no longer prompt 'Reference Error', but prompts 'To exit, press ^D or type .exit'. If the user defines variables named 'exit' or 'quit' , only the value of the variables are output PR-URL: nodejs#20617 Fixes: nodejs#19021 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

BridgeAR · 2018-05-18T13:48:16Z

Landed in 9aa4ec4

@monkingxue congratulations on your first commit to Node.js! 🎉

Imitate python repl, when the user enters 'exit' or 'quit', no longer prompt 'Reference Error', but prompts 'To exit, press ^D or type .exit'. If the user defines variables named 'exit' or 'quit' , only the value of the variables are output PR-URL: #20617 Fixes: #19021 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label May 9, 2018

monkingxue force-pushed the @monkingxue branch from c1c5089 to 4409a66 Compare May 9, 2018 03:52

apapirovski changed the title ~~@monkingxue~~ repl: add hint as to how to exit May 9, 2018

addaleax approved these changes May 9, 2018

View reviewed changes

monkingxue force-pushed the @monkingxue branch from 4409a66 to 1f7312e Compare May 10, 2018 02:47

princejwesley reviewed May 10, 2018

View reviewed changes

BridgeAR approved these changes May 18, 2018

View reviewed changes

monkingxue added 5 commits May 18, 2018 15:37

style: fix the code style

17e1272

fix: duplication of command name and variable name

51282ef

Before output the hint, check if defined the command-named variable as an error

fix: move the check of variables' name to before the command run

93c1647

style

de3385a

monkingxue force-pushed the @monkingxue branch from 7307821 to de3385a Compare May 18, 2018 07:37

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 18, 2018

BridgeAR closed this May 18, 2018

addaleax mentioned this pull request May 22, 2018

v10.2.0 proposal #20724

Merged

jdalton mentioned this pull request May 25, 2018

Repl eval() with context ignored in 10.2.0+ #20965

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repl: add hint as to how to exit #20617

repl: add hint as to how to exit #20617

monkingxue commented May 9, 2018

mscdex commented May 9, 2018 •

edited

Loading

mscdex commented May 9, 2018

monkingxue commented May 9, 2018

monkingxue commented May 9, 2018

monkingxue commented May 9, 2018 •

edited

Loading

Trott commented May 9, 2018

Trott commented May 9, 2018

vsemozhetbyt commented May 9, 2018 •

edited

Loading

vsemozhetbyt commented May 9, 2018

princejwesley commented May 9, 2018

addaleax May 9, 2018

monkingxue May 10, 2018

addaleax May 9, 2018

monkingxue commented May 10, 2018

monkingxue commented May 10, 2018

jasnell commented May 10, 2018

monkingxue commented May 10, 2018

princejwesley commented May 10, 2018 •

edited

Loading

monkingxue commented May 10, 2018

princejwesley May 10, 2018

monkingxue May 11, 2018

princejwesley May 11, 2018

monkingxue May 11, 2018

BridgeAR left a comment

BridgeAR May 18, 2018

BridgeAR May 18, 2018

BridgeAR May 18, 2018

BridgeAR commented May 18, 2018

BridgeAR commented May 18, 2018

		if (err && err.code === 'ERR_SCRIPT_EXECUTION_INTERRUPTED') {
		// The stack trace for this case is not very useful anyway.

repl: add hint as to how to exit #20617

repl: add hint as to how to exit #20617

Conversation

monkingxue commented May 9, 2018

Checklist

mscdex commented May 9, 2018 • edited Loading

mscdex commented May 9, 2018

monkingxue commented May 9, 2018

monkingxue commented May 9, 2018

monkingxue commented May 9, 2018 • edited Loading

Trott commented May 9, 2018

Trott commented May 9, 2018

vsemozhetbyt commented May 9, 2018 • edited Loading

vsemozhetbyt commented May 9, 2018

princejwesley commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monkingxue commented May 10, 2018

monkingxue commented May 10, 2018

jasnell commented May 10, 2018

monkingxue commented May 10, 2018

princejwesley commented May 10, 2018 • edited Loading

monkingxue commented May 10, 2018

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

BridgeAR left a 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

BridgeAR commented May 18, 2018

BridgeAR commented May 18, 2018

mscdex commented May 9, 2018 •

edited

Loading

monkingxue commented May 9, 2018 •

edited

Loading

vsemozhetbyt commented May 9, 2018 •

edited

Loading

princejwesley commented May 10, 2018 •

edited

Loading