repl: allow multiline function call #3823

Zirak · 2015-11-14T11:50:21Z

Currently, the repl allows multiline function declarations, strings, and
all sorts of niceties by catching the SyntaxErrors they issue and
ignoring them. However, the SyntaxError raised by multiline function
calls was not caught. This commit adds to the whitelist.

Behaviour before this commit:

> console.log(console.log(1,
... 2)
SyntaxError: missing ) after argument list

After:

> console.log(console.log(1,
... 2),
... 3)
1 2
undefined 3
undefined

bnoordhuis · 2015-11-14T12:07:35Z

lib/repl.js

@@ -1126,7 +1126,7 @@ function isRecoverableError(e, self) {
      self._inTemplateLiteral = true;
      return true;
    }
-    return /^(Unexpected end of input|Unexpected token)/.test(message);
+    return /^(Unexpected end of input|Unexpected token|missing \) after argument list)/.test(message);


Can you wrap this at 80 columns?

As much as it pains me, the indentation + the regexp itself are at 80 characters, and multiline regexp isn't a thing in js.
It can be made a little shorter by moving the regexp to a variable, or if you'd rather I can cook something up which doesn't use regexps, maybe:

var prefixes = [ 'Unexpected end of input', // ... ]; return prefixes.some(message.startsWith.bind(message));

I know, but you can break it up into multiple regular expressions or compile one dynamically with new RegExp(...). I'd take the multi-regex approach.

@Zirak Perhaps, this?

const recoverableErrors = ['Unexpected end of input', 'Unexpected token', 'missing \\) after argument list' ]; const recoverableErrorsRegEx = new RegExp(`^(${recoverableErrors.join('|')})`); ... return recoverableErrorsRegEx.test(message);

@thefourtheye hrngh, I thought about new RegExp, but every time I do something like that some paranoid part of me worries about escaping.

Multiple regexps (or the array one I posted above) is a bit more verbose, but may be better in that regard.

@Zirak mmm, then lets do it as multiple regexps. I am actually eager to see that solution, as I didn't understand it :D

bnoordhuis · 2015-11-14T12:07:45Z

LGTM apart from a style issue.

thefourtheye · 2015-11-14T12:24:14Z

test/parallel/test-repl.js

+    { client: client_unix, send: '2)',
+      expect: prompt_multiline },
+    { client: client_unix, send: ')',
+      expect: 'undefined' },


I think prompt_unix should immediately follow the undefined. Can you please test and include that as well?

The last send closes off the outer function call and prints the output, so there's no prompt_multiline prepending the result.

@Zirak prompt_multiline is ... and prompt_unix is the > which we get in the REPL.

@thefourtheye huh, that's weird. I (of course) ran the tests as committed and they pass, but expecting a 'undefined\n' + prompt_unix also passed the test. I'll edit that in as you requested.

jasnell · 2015-11-14T15:39:29Z

semver-minor only because it's technically adding new capability, not fixing a bug. Very good fix tho.
LGTM.

Zirak · 2015-11-14T21:04:23Z

@bnoordhuis, @thefourtheye Should be fixed now, I went with the "dumb" way of doing it, following the example of the previous lines. Just list them out, nothing fancy, nothing special.

Meta-question, is this the proper way of fixing the PR, or should it be squashed?

jasnell · 2015-11-16T15:53:34Z

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

jasnell · 2015-11-16T15:55:50Z

@Zirak .. either way, really. While a PR is still being reviewed, there's no reason to squash -- in fact, in many cases keeping them separate helps with the review. Before landing, the commits ought to be squashed... which can be done by the person landing the PR but ideally ought to be done by the submitter (simply because it's helpful, I'll typically squash commits when I land tho)

thefourtheye · 2015-11-16T15:57:37Z

@Zirak yes. The commits have to be squashed. If you can do it, it would be awesome. Otherwise we can do it while landing

jasnell · 2015-11-18T05:49:04Z

lib/repl.js

+
+    return message.startsWith('Unexpected end of input') ||
+      message.startsWith('Unexpected token') ||
+      message.startsWith('missing ) after argument list');
  }


Perhaps a better approach would be defining a const as:

const remsg = new RegExp( '^(Unexpected end of input|' + 'Unexpected token|' + 'missing \\) after argument list)' );

and using return remsg.test(message); ?

Yup. We had discussed that in one of the old comments :)

Oh lol. Sorry, missed that entirely ;)
On Nov 17, 2015 10:28 PM, "thefourtheye" notifications@github.com wrote:

In lib/repl.js
#3823 (comment):

@@ -1126,7 +1126,10 @@ function isRecoverableError(e, self) {
self._inTemplateLiteral = true;
return true;
}

return /^(Unexpected end of input|Unexpected token)/.test(message);

return message.startsWith('Unexpected end of input') ||

message.startsWith('Unexpected token') ||

message.startsWith('missing ) after argument list');
}

Yup. We had discussed that in one of the old comments :)

—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/3823/files#r45164076.

No problem :)

@jasnell I spent some time thinking about which way to go about this, and at the end decided with the "dumb" way. Doing it in fancy ways is possible, but doesn't give us readability or extensibility or anything else. So I fought the urge to rewrite that part.

...even though it's so tempting.

thefourtheye · 2016-02-14T07:58:54Z

LGTM. CI Run: https://ci.nodejs.org/job/node-test-pull-request/1655/

jasnell · 2016-02-15T15:38:40Z

There was an odd build bot failure on OSX.. running CI again: https://ci.nodejs.org/job/node-test-pull-request/1663/

Currently, the repl allows multiline function declarations, strings, and all sorts of niceties by catching the SyntaxErrors they issue and ignoring them. However, the SyntaxError raised by multiline function calls was not caught. This commit adds to the whitelist.

Zirak · 2016-02-16T08:26:55Z

@thefourtheye Squashed the commits into one.

Currently, the repl allows multiline function declarations, strings, and all sorts of niceties by catching the SyntaxErrors they issue and ignoring them. However, the SyntaxError raised by multiline function calls was not caught. This commit adds to the whitelist. PR-URL: #3823 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

thefourtheye · 2016-02-16T08:36:30Z

Thanks @Zirak :-) Landed in a06066c

Currently, the repl allows multiline function declarations, strings, and all sorts of niceties by catching the SyntaxErrors they issue and ignoring them. However, the SyntaxError raised by multiline function calls was not caught. This commit adds to the whitelist. PR-URL: #3823 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Currently, the repl allows multiline function declarations, strings, and all sorts of niceties by catching the SyntaxErrors they issue and ignoring them. However, the SyntaxError raised by multiline function calls was not caught. This commit adds to the whitelist. PR-URL: nodejs#3823 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

cjihrig · 2016-02-25T21:22:31Z

This appears to be an error that we weren't properly handling before. I'd like to float the idea of this as semver-patch instead of semver-minor, unless someone has strong feelings otherwise.

Refs: #5437

r-52 added the repl Issues and PRs related to the REPL subsystem. label Nov 14, 2015

bnoordhuis reviewed Nov 14, 2015
View reviewed changes

thefourtheye reviewed Nov 14, 2015
View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 14, 2015

jasnell reviewed Nov 18, 2015
View reviewed changes

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:01

Zirak force-pushed the repl-arglist branch from 3c396aa to 56a433b Compare February 16, 2016 08:26

thefourtheye closed this Feb 16, 2016

tomgco mentioned this pull request Feb 16, 2016

repl: Allow fn parenthesis across multiple lines #4585

Closed

princejwesley mentioned this pull request Feb 25, 2016

REPL syntax error when closing bracket & parenthesis are on different lines #4060

Closed

cjihrig mentioned this pull request Feb 25, 2016

SyntaxError bug : Coding style problem #5437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repl: allow multiline function call #3823

repl: allow multiline function call #3823

Zirak commented Nov 14, 2015

bnoordhuis Nov 14, 2015

Zirak Nov 14, 2015

bnoordhuis Nov 14, 2015

thefourtheye Nov 14, 2015

Zirak Nov 14, 2015

thefourtheye Nov 14, 2015

bnoordhuis commented Nov 14, 2015

thefourtheye Nov 14, 2015

Zirak Nov 14, 2015

thefourtheye Nov 14, 2015

Zirak Nov 14, 2015

jasnell commented Nov 14, 2015

Zirak commented Nov 14, 2015

jasnell commented Nov 16, 2015

jasnell commented Nov 16, 2015

thefourtheye commented Nov 16, 2015

jasnell Nov 18, 2015

thefourtheye Nov 18, 2015

jasnell Nov 18, 2015

thefourtheye Nov 18, 2015

Zirak Nov 18, 2015

thefourtheye commented Feb 14, 2016

jasnell commented Feb 15, 2016

Zirak commented Feb 16, 2016

thefourtheye commented Feb 16, 2016

cjihrig commented Feb 25, 2016

repl: allow multiline function call #3823

repl: allow multiline function call #3823

Conversation

Zirak commented Nov 14, 2015

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

bnoordhuis commented Nov 14, 2015

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

jasnell commented Nov 14, 2015

Zirak commented Nov 14, 2015

jasnell commented Nov 16, 2015

jasnell commented Nov 16, 2015

thefourtheye commented Nov 16, 2015

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

thefourtheye commented Feb 14, 2016

jasnell commented Feb 15, 2016

Zirak commented Feb 16, 2016

thefourtheye commented Feb 16, 2016

cjihrig commented Feb 25, 2016