Skip to content

Don't strip spaces for regular REPL input - #2246

Closed
edef1c wants to merge 1 commit into
nodejs:masterfrom
edef1c:fix-repl-dots
Closed

Don't strip spaces for regular REPL input#2246
edef1c wants to merge 1 commit into
nodejs:masterfrom
edef1c:fix-repl-dots

Conversation

@edef1c

Copy link
Copy Markdown
Contributor

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`.
@edef1cedef1c changed the title don't strip spaces for regular REPL inputDon't strip spaces for regular REPL inputJul 25, 2015
@edef1cedef1c closed this Jul 25, 2015
@edef1cedef1c reopened this Jul 25, 2015
@edef1c

Copy link
Copy Markdown
ContributorAuthor

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)
> 

@brendanashworthbrendanashworth added the repl Issues and PRs related to the REPL subsystem. label Jul 25, 2015
@silverwind

Copy link
Copy Markdown
Contributor

cc @thefourtheye

@Fishrock123

Copy link
Copy Markdown
Contributor

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

@Fishrock123Fishrock123 added this to the 2.5.0 milestone Jul 25, 2015
@edef1c

Copy link
Copy Markdown
ContributorAuthor

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

@thefourtheye

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@thefourtheye

Copy link
Copy Markdown
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?

@Fishrock123Fishrock123 removed this from the 2.5.0 milestone Jul 27, 2015
@targos

Copy link
Copy Markdown
Member

@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.

@Fishrock123Fishrock123 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
Copy Markdown
Member

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

@jasnelljasnell added the stalled Issues and PRs that are stalled. label Nov 16, 2015
@princejwesley

Copy link
Copy Markdown
Contributor

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

@thefourtheye

Copy link
Copy Markdown
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
Copy Markdown
Contributor

@jasnell@thefourtheye#3835 fixed this issue.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

confirmed-bugIssues with confirmed bugs.replIssues and PRs related to the REPL subsystem.stalledIssues and PRs that are stalled.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@edef1c@silverwind@Fishrock123@thefourtheye@targos@jasnell@princejwesley@brendanashworth