repl: better REPL commands detection

[thefourtheye]

Fixes the problem shown in #2246

The REPL module, treats any line which starts with a dot character as
a REPL command and this limits the ability to use function calls in
the next lines to improve readability.

This patch checks if the current line starts with . and then a series
of lowercase letters and then any characters.

For example:

> process.version 'v2.4.0' > function sum(arr){... return arr ... .reduce(function(c, x){return x + c}); Invalid REPL keyword undefined 

but then, this patches allows this:

> process.version 'v2.4.1-pre' > function sum(arr){... return arr ... .reduce(function(c, x){return x + c}); ... } undefined 

[Fishrock123]

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/204/

[thefourtheye]

Right now I am thinking about using a different character instead of ., say %, to represent REPL commands.

[Fishrock123]

~command? That's kind of a breaking API change though.. maybe we should leave that for a different PR/issue?

[thefourtheye]

@Fishrock123 You mean like .break-command? But that would be too much to type, no? That is why I thought of %, which cannot be in a valid JS identifier.

[Fishrock123]

Doh, true, ~ is bitwise inverse.

[cjihrig]

I think starting with % could be a problem in the edge case that the REPL is run with the --allow-native-syntax flag. It's much less likely to cause problems than the . though. And of course the whole breaking change thing.

EDIT: Oh, and the modulus operator

[thefourtheye]

Oh yeah. That is also correct. # then?

[cjihrig]

I don't think changing characters is necessarily the best idea. We've already committed to ., and what if # becomes significant one day? Would it be enough to recognize that the REPL is in the middle of an expression and not try to interpret REPL commands?

[thefourtheye]

Hmmm, true. So, breakage is unavoidable, no matter what we actually choose as the REPL command identifier.

[targos]

@cjihrig We cannot just ignore REPL commands in an expression, especially .break that is here to get out of an unfinished expression AFAIK

[cjihrig]

I think you can, you just need the correct context, much like you must be able to determine the context of the / character (is it division, a regular expression, or a comment). If you say foo., it should never be interpreted as a REPL command, regardless of whitespace.

[targos]

If course, but don't we need to introduce some kind of JS parsing to do that ?

[cjihrig]

I don't think we need a full blown parser, grammar and all. I think we just need to maintain a bit more state (like the existing isWithinStrLiteral, _inTemplateLiteral, etc.). Also, not volunteering to do this work :-)

[thefourtheye]

I can take that up, but I am not sure if it is really necessary. As of now, I am just happy with this RegEx (though I have a feeling that this might break if the filename has ( with .load or .save

[Fishrock123]

(though I have a feeling that this might break if the filename has ( with .load or .save)

Sounds like a test case!

[thefourtheye]

@Fishrock123 Oh yeah :-) I'll include that, but is it okay to land this fix with that known limitation?

[thefourtheye]

Rebased and incorporated the test case suggested by @Fishrock123 :-)

CI Run: node-test-pull-request/70

[thefourtheye]

And the CI is green (failing cases are not related to this change) :-)

[thefourtheye]

Bump!

[thefourtheye]

@Fishrock123 Ping!

[Fishrock123]

I think the parse regex is far too lenient to begin with, and that we could probably only use one regex. Furthermore, why don't we just put this within if (!wasWithinStrLiteral && !isWithinStrLiteral){ above? That way we can catch it before anything is trimmed.

I'm suggesting something like:

if(!wasWithinStrLiteral&&!isWithinStrLiteral){if(cmd&&isNaN(parseFloat(cmd)){constmatches=/^\.[a-z-_]+\s*[^(){}[];=|&]*$/.exec(cmd); ... }cmd=cmd.trim();}

as an aside, we should probably just check before everything if (\^\s*$\.test(cmd)) (or (cmd.trim() === '')

[thefourtheye]

/^\.[a-z-_]+\s*[^(){}[];=|&]*$/ doesn't allow file names with the special characters^(){}[];=|&. I can live that. Also _ and - are not necessary in it, because the REPL commands don't have them.

[Fishrock123]

Also _ and - are not necessary in it, because the REPL commands don't have them.

The defaults don't currently, correct. But they could eventually. Also I think the repl can be extended to add more commands? If so, those characters seem like likely candidates to exist in a custom command. While we are at it we should add A-Z to the match too.

Given the above, it's possible this would technically be a breaking change then? I'm not so sure, or concerned either.

[thefourtheye]

Hmmm, looks like this would be very tricky to fix and will still break from time to time :'(