repl: handle comments properly by thefourtheye · Pull Request #3515 · nodejs/node

thefourtheye · 2015-10-25T18:21:28Z

As it is, the comments are not handled properly in REPL. So, if the
comments have ' or ", then they are treated as string literals and
the error is thrown in REPL.

This patch refactors the existing logic and groups everything in a
class.

Fixes: #3421

cc @mscdex @silverwind @ChuckLangford @Fishrock123

mscdex · 2015-10-25T19:04:09Z

CI: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/609/

LGTM

ChuckLangford · 2015-10-26T01:09:40Z

The code changes LGTM.
This being my first core review I have a question, how detailed do we get with tests? The test on line 260 is good but I don't see the same test for a single quote.

@ChuckLangford Thanks for the feedback. I thought I covered it in 254, but we have both tests at 257 and 260. More tests is good I guess :-) I included that now. PTAL.

As it is, the comments are not handled properly in REPL. So, if the comments have `'` or `"`, then they are treated as string literals and the error is thrown in REPL. This patch refactors the existing logic and groups everything in a class. Fixes: nodejs#3421

targos · 2015-10-26T07:19:22Z

suggestion: use a for...of loop

Oh yeah, I'll change it.

thefourtheye · 2015-10-26T07:35:04Z

@targos I modified the PR as per your suggestions. PTAL.

ChuckLangford · 2015-10-26T11:28:19Z

LGTM

Fishrock123 · 2015-10-26T17:24:04Z

This is for (... of ...) on a String? I was unaware that worked.

@Fishrock123 It actually works.
'use strict';for(constchrof"abcd"){console.log(chr);}

It works and has the advantage to handle unicode correctly:

Fishrock123 · 2015-10-26T17:28:21Z

Seems fine so far to me.

Fishrock123 · 2015-10-26T17:53:28Z

LGTM if it works

thefourtheye · 2015-10-27T12:08:54Z

Yet another CI run: https://ci.nodejs.org/job/node-test-pull-request/626/

jasnell · 2015-10-27T17:15:33Z

semver-minor?

thefourtheye · 2015-10-27T17:44:21Z

@jasnell Hmmm, wouldn't this be just a bug fix? Also, can we backport this to 4.x LTS? People who use REPL will be impacted.

jasnell · 2015-10-27T17:49:31Z

Yes and no. If it's, "Previously the REPL didn't support comments, now it does", then it's a new feature, semver-minor, and should not land in v4.x. However, if it's, "REPL was always supposed to support comments and we consider this a regression", then a case can be made for landing it in v4.x.

Fishrock123 · 2015-10-27T17:56:20Z

If it helps I think this should be fixed in v4.x.

jasnell · 2015-10-27T18:18:06Z

It does help. Getting some others from the @nodejs/tsc and @nodejs/lts teams to chime in would help also. I'm leaning towards a yes for v4.x also.

silverwind · 2015-10-27T18:18:10Z

I don't see this as semver-worthy. The expectation is that you can paste any JS into the REPL, so this is a 'feature' that should've been there from the start. Also, I'd be surprised if this is not actually a regression.

As it is, the comments are not handled properly in REPL. So, if the comments have `'` or `"`, then they are treated as incomplete string literals and the error is thrown in REPL. This patch refactors the existing logic and groups everything in a class. Fixes: #3421 PR-URL: #3515 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

thefourtheye · 2015-10-28T02:03:02Z

Thanks for the review people. Landed at 6cf1910

As it is, the comments are not handled properly in REPL. So, if the comments have `'` or `"`, then they are treated as incomplete string literals and the error is thrown in REPL. This patch refactors the existing logic and groups everything in a class. Fixes: #3421 PR-URL: #3515 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell · 2015-10-28T15:31:24Z

Landed in v4.x-staging in d918838

As it is, the comments are not handled properly in REPL. So, if the comments have `'` or `"`, then they are treated as incomplete string literals and the error is thrown in REPL. This patch refactors the existing logic and groups everything in a class. Fixes: nodejs#3421 PR-URL: nodejs#3515 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

As it is, the comments are not handled properly in REPL. So, if the comments have `'` or `"`, then they are treated as incomplete string literals and the error is thrown in REPL. This patch refactors the existing logic and groups everything in a class. Fixes: #3421 PR-URL: #3515 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

thefourtheye added the repl Issues and PRs related to the REPL subsystem. label Oct 25, 2015

thefourtheye mentioned this pull request Oct 25, 2015
repl hates comments with quotes in indented code #3421
Closed

thefourtheye force-pushed the fix-comments-in-repl branch 2 times, most recently from 67fa425 to 8179911Compare October 25, 2015 18:33

ChuckLangford reviewed Oct 26, 2015
View reviewed changes

thefourtheye force-pushed the fix-comments-in-repl branch from 8179911 to 5f656dbCompare October 26, 2015 02:23

targos reviewed Oct 26, 2015
View reviewed changes

addressing targos's comments
60aeee5

Fishrock123 reviewed Oct 26, 2015
View reviewed changes

jasnell added the lts-watch-v4.x label Oct 27, 2015

thefourtheye closed this Oct 28, 2015

thefourtheye deleted the fix-comments-in-repl branch October 28, 2015 02:03

jasnell added land-on-v4.x and removed lts-watch-v4.x labels Oct 28, 2015

jasnell mentioned this pull request Oct 28, 2015
Preparing v4.2.2 LTS Maintenance nodejs/Release#50
Closed

rvagg mentioned this pull request Oct 29, 2015
Release proposal: v5.0.0 #3466
Merged

jasnell mentioned this pull request Oct 29, 2015
Release Proposal: v4.2.2 "Argon" (LTS) #3588
Closed

jasnell removed the land-on-v4.x label Nov 3, 2015

This was referenced Apr 29, 2023
[Snyk] Security upgrade karma from 3.1.4 to 5.0.8 aliscco/alisco-node#375
Open
[Snyk] Security upgrade karma from 3.1.4 to 5.0.8 aliscco/alisco-node#379
Open

leonardoadame mentioned this pull request Jul 9, 2023
[Snyk] Security upgrade karma from 3.1.4 to 5.0.8 leonardoadame/node#143
Open

aliscco mentioned this pull request Oct 27, 2023
[Snyk] Security upgrade karma from 3.1.4 to 5.0.8 aliscco/alisco-node#646
Open

aliscco mentioned this pull request Nov 28, 2023
[Snyk] Security upgrade karma from 3.1.4 to 5.0.8 aliscco/alisco-node#695
Open

This was referenced Nov 29, 2023
[Snyk] Security upgrade karma from 3.1.4 to 5.0.8 leonardoadame/node#211
Open
[Snyk] Security upgrade karma from 3.1.4 to 5.0.8 leonardoadame/node#268
Open

aliscco mentioned this pull request Dec 2, 2023
[Snyk] Security upgrade karma from 3.1.4 to 5.0.8 aliscco/alisco-node#1024
Open

leonardoadame mentioned this pull request Dec 3, 2023
[Snyk] Security upgrade karma from 3.1.4 to 5.0.8 leonardoadame/node#479
Open

Uh oh!

Uh oh!

Conversation

thefourtheye commented Oct 25, 2015

Uh oh!

mscdex commented Oct 25, 2015

Uh oh!

ChuckLangfordOct 26, 2015

Choose a reason for hiding this comment

Uh oh!

thefourtheyeOct 26, 2015

Choose a reason for hiding this comment

Uh oh!

targosOct 26, 2015

Choose a reason for hiding this comment

Uh oh!

thefourtheyeOct 26, 2015

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Oct 26, 2015

Uh oh!

ChuckLangford commented Oct 26, 2015

Uh oh!

Fishrock123Oct 26, 2015

Choose a reason for hiding this comment

Uh oh!

thefourtheyeOct 26, 2015

Choose a reason for hiding this comment

Uh oh!

targosOct 26, 2015

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Oct 26, 2015

Uh oh!

Fishrock123 commented Oct 26, 2015

Uh oh!

thefourtheye commented Oct 27, 2015

Uh oh!

jasnell commented Oct 27, 2015

Uh oh!

thefourtheye commented Oct 27, 2015

Uh oh!

jasnell commented Oct 27, 2015

Uh oh!

Fishrock123 commented Oct 27, 2015

Uh oh!

jasnell commented Oct 27, 2015

Uh oh!

silverwind commented Oct 27, 2015

Uh oh!

thefourtheye commented Oct 28, 2015

Uh oh!

jasnell commented Oct 28, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants