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

Don't strip spaces for regular REPL input #2246

Closed
wants to merge 1 commit into from
Closed

Don't strip spaces for regular REPL input #2246

wants to merge 1 commit into from

Conversation

edef1c
Copy link
Contributor

@edef1c edef1c commented Jul 25, 2015

This means .method() isn't misparsed as a REPL keyword, and significantly improves the usability of .load.

This means `  .method()` isn't misparsed as a REPL keyword,
and significantly improves the usability of `.load`.
@edef1c edef1c changed the title don't strip spaces for regular REPL input Don't strip spaces for regular REPL input Jul 25, 2015
@edef1c edef1c closed this Jul 25, 2015
@edef1c edef1c reopened this Jul 25, 2015
@edef1c
Copy link
Contributor Author

edef1c commented Jul 25, 2015

Expected (and this patch's) behaviour:

> .load test.js
> function sum(arr) {
...   return arr
...     .reduce(function(acc, x) { return acc + x }, 0)
... }
undefined
> console.log(sum([20, 1, 21]))
42
undefined
> 

Current behaviour:

> .load test.js
> function sum(arr) {
...   return arr
...     .reduce(function(acc, x) { return acc + x }, 0)
Invalid REPL keyword
undefined
> }
SyntaxError: Unexpected token }
    at Object.exports.createScript (vm.js:24:10)
    at REPLServer.defaultEval (repl.js:131:25)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:308:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:209:10)
    at REPLServer.Interface._line (readline.js:548:8)
    at REPLServer.Interface._ttyWrite (readline.js:879:20)
> console.log(sum([20, 1, 21]))
ReferenceError: sum is not defined
    at repl:1:13
    at REPLServer.defaultEval (repl.js:154:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:308:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:209:10)
    at REPLServer.Interface._line (readline.js:548:8)
    at REPLServer.Interface._ttyWrite (readline.js:879:20)
> 

@brendanashworth brendanashworth added the repl Issues and PRs related to the REPL subsystem. label Jul 25, 2015
@silverwind
Copy link
Contributor

cc @thefourtheye

@Fishrock123
Copy link
Contributor

Oops, probably my fault. Hmm, will need to fix this before the 2.5.0 release.

@Fishrock123 Fishrock123 added this to the 2.5.0 milestone Jul 25, 2015
@edef1c
Copy link
Contributor Author

edef1c commented Jul 25, 2015

@Fishrock123 Nah, this has been the case for years.

@thefourtheye
Copy link
Contributor

Hmmm. I knew about this weak REPL command check when I submitted REPL bug fixes PR, but I could not come up with a practical usecase. So, apart from removing the check mentioned in this PR, it would be better if we improved the REPL command check with a regular expression. Instead of

cmd && cmd.charAt(0) === '.' && isNaN(parseFloat(cmd))

if we had

cmd && isNaN(parseFloat(cmd)) && /.[a-z]+\s*[a-z]*/.test(cmd)

it would be better. Also, I would recommend including a test as well.

Edit: To me, stripping spaces is not the actual problem here (even though leaving the spaces when we are not inside a string literal is not a big deal), but considering the current line of input as a REPL command just because it starts with . doesn't seem right. That is why I proposed the RegEx above.

Edit 2: It would be better if the RegEx is /^.[a-z]+\s*[\w]*$/.test(cmd)

@targos
Copy link
Member

targos commented Jul 25, 2015

@thefourtheye you can easily run into this issue by copy-pasting something in the REPL

@thefourtheye
Copy link
Contributor

@targos Yup, I understand that after seeing the example shown in the PR. That is why using a RegEx to validate the string should work here. What do you think?

@Fishrock123 Fishrock123 removed this from the 2.5.0 milestone Jul 27, 2015
@targos
Copy link
Member

targos commented Jul 27, 2015

@thefourtheye Yes a RegExp could be the solution. We just need to make sure that it does not identify too much valid JS as a REPL command.

@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label Jul 27, 2015
thefourtheye added a commit to thefourtheye/io.js that referenced this pull request Aug 11, 2015
Fixes the problem shown in nodejs#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
@jasnell
Copy link
Member

jasnell commented Nov 16, 2015

@nathan7 @thefourtheye @Fishrock123 ... what's the status on this one?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@princejwesley
Copy link
Contributor

@Fishrock123 @thefourtheye /^\.[a-z][^(]*$/ is sufficient, right?

@thefourtheye
Copy link
Contributor

@princejwesley I proposed almost the same in #2254. But the problem is, load and save accept filenames as inputs following them. If they had ( in them then this solution will break.

@princejwesley
Copy link
Contributor

@jasnell @thefourtheye #3835 fixed this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. repl Issues and PRs related to the REPL subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants