repl: handle comments properly#3515
Uh oh!
There was an error while loading. Please reload this page.
Conversation
thefourtheyeforce-pushed the fix-comments-in-repl branch 2 times, most recently from 67fa425 to 8179911Compare
mscdex commented Oct 25, 2015
test/parallel/test-repl.js Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
thefourtheyeforce-pushed the fix-comments-in-repl branch from 8179911 to 5f656dbCompare
lib/repl.js Outdated
There was a problem hiding this comment.
Oh yeah, I'll change it.
thefourtheye commented Oct 26, 2015
@targos I modified the PR as per your suggestions. PTAL. |
ChuckLangford commented Oct 26, 2015
LGTM |
There was a problem hiding this comment.
This is for (... of ...) on a String? I was unaware that worked.
There was a problem hiding this comment.
@Fishrock123 It actually works.
'use strict';for(constchrof"abcd"){console.log(chr);}There was a problem hiding this comment.
It works and has the advantage to handle unicode correctly:
Fishrock123 commented Oct 26, 2015
Seems fine so far to me. |
Fishrock123 commented Oct 26, 2015
LGTM if it works |
thefourtheye commented Oct 27, 2015
Yet another CI run: https://ci.nodejs.org/job/node-test-pull-request/626/ |
jasnell commented Oct 27, 2015
semver-minor? |
thefourtheye commented Oct 27, 2015
@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 commented Oct 27, 2015
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 |
Fishrock123 commented Oct 27, 2015
If it helps I think this should be fixed in v4.x. |
jasnell commented Oct 27, 2015
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 commented Oct 27, 2015
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 <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
thefourtheye commented Oct 28, 2015
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 <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
jasnell commented Oct 28, 2015
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 <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
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 <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>
7 participants
As it is, the comments are not handled properly in REPL. So, if the
comments have
'or", then they are treated as string literals andthe error is thrown in REPL.
This patch refactors the existing logic and groups everything in a
class.
Fixes: #3421
cc @mscdex@silverwind@ChuckLangford@Fishrock123