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

repl: do not consider ... as a REPL command #14467

Closed
wants to merge 5 commits into from

Conversation

shivanth
Copy link
Contributor

@shivanth shivanth commented Jul 25, 2017

This fix makes ... in REPL to be considered as a javascript construct
rather than a REPL keyword

Fixes: #14426

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

This fix makes ... in REPL to be considered as a javascript construct
rather than a REPL keyword

Fixes: nodejs#14426
@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Jul 25, 2017
Trott
Trott previously requested changes Jul 25, 2017
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Please add a test! REPL tests can be tricky, but this is probably something that can be tested by adding something to test/parallel/test-repl.js. Additional info about our tests in general can be found in https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md

@silverwind
Copy link
Contributor

Change looks good. For adding a test, check out https://github.com/nodejs/node/blob/master/test/parallel/test-repl.js.

lib/repl.js Outdated
@@ -419,7 +419,7 @@ function REPLServer(prompt,
// Check to see if a REPL keyword was used. If it returns true,
// display next prompt and return.
if (trimmedCmd) {
if (trimmedCmd.charAt(0) === '.' && isNaN(parseFloat(trimmedCmd))) {
if (trimmedCmd.charAt(0) === '.' && trimmedCmd.charAt(1) != '.' && isNaN(parseFloat(trimmedCmd))) {
Copy link
Member

@TimothyGu TimothyGu Jul 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use strict not equals, so trimmedCmd.charAt(1) !== '.'

@mscdex
Copy link
Contributor

mscdex commented Jul 25, 2017

FWIW commit message is missing a space after the colon.

@refack refack changed the title repl:vDo not consider ... as a REPL command repl: do not consider ... as a REPL command Jul 25, 2017
@refack
Copy link
Contributor

refack commented Jul 25, 2017

@shivanth thank for your contribution 🥇. Don't be alarmed by all the reviews, as far as I can see they are just in order to make your submission even better. I would really want to see you follow up, so this PR will land.

@refack
Copy link
Contributor

refack commented Jul 25, 2017

P.S. as far as I can see this change also enables ..* to be treated as a non REPL command. IMHO that's good since it's just as invalid as ...[] 👍 So when you add a test, try that as well.

@shivanth
Copy link
Contributor Author

@refack The ...command seems to put the repl into an inconsistent state, it always shows up a multiline command, which can only be exited by pressing ctrl-c.


> ..save
... dad
... ()
... blah
...
>

I came across this when I tried to add a new test case

@silverwind
Copy link
Contributor

The...command seems to put the repl into an inconsistent state

That's something that should be investigated separately. Any kind of invalid syntax does it, but is has its uses too:

> a=
... 1;
1

Same also works on the shell:

$ node -p "a=\
dquote> 1"
1

The question is if it can be determined if a line can never be valid, like ...something, so we don't show the line continuation and output the error immediately, but I guess this can get complex.

As a first step, I'm fine if you just make sure it not gets parsed as a REPL command.

@shivanth
Copy link
Contributor Author

Because my test case is leaving the REPL in an inconsistent state, the tests that follow my new test fails ...

sending "...[]"
Unix data: "... ", expecting "... "
sending "ref = 1"
Unix data: "... ", expecting /^ReferenceError:\sref\sis\snot\sdefined\n\s+at\srepl:1:5/
assert.js:43
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: '... ' === /^ReferenceError:\sref\sis\snot\sdefined\n\s+at\srepl:1:5/

@shivanth
Copy link
Contributor Author

Done 👍

@shivanth
Copy link
Contributor Author

@Trott @TimothyGu

@Trott
Copy link
Member

Trott commented Jul 27, 2017

The test as it stands right now does not fail on current master so it is not testing the feature implemented here.

@Trott Trott dismissed their stale review July 27, 2017 20:47

test added, dismissing review

@Trott
Copy link
Member

Trott commented Jul 27, 2017

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -414,7 +414,14 @@ function error_test() {
expect: `${prompt_multiline}'foo \\n'\n${prompt_unix}` },
// Whitespace is not evaluated.
{ client: client_unix, send: ' \t \n',
expect: prompt_unix }
expect: prompt_unix },
//Do not parse `...[]` as a REPL keyword
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the linter might fail due to no space between the comment start?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It passes as we do not use the spaced-comment rule. I fixed it anyways.

@silverwind
Copy link
Contributor

Thanks, landed in 46d3ff2!

I fixed the whitespace issues in the test and wrapped the long line in lib so it passed the linter.

@silverwind silverwind closed this Jul 29, 2017
addaleax pushed a commit that referenced this pull request Jul 29, 2017
This fix makes ... in REPL to be considered as a javascript construct
rather than a REPL keyword.

Fixes: #14426
PR-URL: #14467
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@addaleax addaleax mentioned this pull request Aug 2, 2017
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@silverwind
Copy link
Contributor

I'd say so. @shivanth wanna do it?

@shivanth
Copy link
Contributor Author

I'm in 👍

shivanth added a commit to shivanth/node that referenced this pull request Aug 18, 2017
This fix makes ... in REPL to be considered as a javascript construct
rather than a REPL keyword.

Fixes: nodejs#14426
PR-URL: nodejs#14467
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 19, 2017
This fix makes ... in REPL to be considered as a javascript construct
rather than a REPL keyword.

Fixes: #14426
Backport-PR-URL: #14915
PR-URL: #14467
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
@0joshuaolson1
Copy link

Was an issue ever created for the aforementioned problem where some syntax errors put the repl in an 'inconsistent' ... state until you Ctrl+C?

I found #18915, but its a PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

repl: '...' should not be detected as REPL keyword