FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ node Public

repl: handle comments properly - #3515

Closed
thefourtheye wants to merge 2 commits into
nodejs:masterfrom
thefourtheye:fix-comments-in-repl
ConversationCommits (2)ChecksFiles changed
Closed

repl: handle comments properly#3515
thefourtheye wants to merge 2 commits into
nodejs:masterfrom
thefourtheye:fix-comments-in-repl

Conversation

thefourtheye commented Oct 25, 2015

Copy link
Copy Markdown
Contributor

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

thefourtheye added the repl Issues and PRs related to the REPL subsystem. label Oct 25, 2015
thefourtheye mentioned this pull request Oct 25, 2015
Closed
thefourtheye force-pushed the fix-comments-in-repl branch 2 times, most recently from 67fa425 to 8179911 Compare October 25, 2015 18:33

mscdex commented Oct 25, 2015

Copy link
Copy Markdown
Contributor

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

LGTM

ChuckLangford reviewed Oct 26, 2015
View reviewed changes
Comment thread test/parallel/test-repl.js Outdated

ChuckLangford Oct 26, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

thefourtheye Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

repl: handle comments properly
5f656db 
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 reviewed Oct 26, 2015
View reviewed changes
Comment thread lib/repl.js Outdated

targos Oct 26, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

suggestion: use a for...of loop

thefourtheye Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh yeah, I'll change it.

thefourtheye commented Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

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

ChuckLangford commented Oct 26, 2015

Copy link
Copy Markdown
Contributor

LGTM

Fishrock123 reviewed Oct 26, 2015
View reviewed changes
Comment thread lib/repl.js

Fishrock123 Oct 26, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

thefourtheye Oct 26, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Fishrock123 It actually works.

'use strict';

for (const chr of "abcd") {
  console.log(chr);
}

targos Oct 26, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It works and has the advantage to handle unicode correctly:

Fishrock123 commented Oct 26, 2015

Copy link
Copy Markdown
Contributor

Seems fine so far to me.

Fishrock123 commented Oct 26, 2015

Copy link
Copy Markdown
Contributor

LGTM if it works

thefourtheye commented Oct 27, 2015

Copy link
Copy Markdown
Contributor Author

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

jasnell commented Oct 27, 2015

Copy link
Copy Markdown
Member

semver-minor?

thefourtheye commented Oct 27, 2015

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

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 commented Oct 27, 2015

Copy link
Copy Markdown
Contributor

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

jasnell commented Oct 27, 2015

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor

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.

thefourtheye added a commit that referenced this pull request Oct 28, 2015
repl: handle comments properly
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>

thefourtheye commented Oct 28, 2015

Copy link
Copy Markdown
Contributor Author

Thanks for the review people. Landed at 6cf1910

thefourtheye deleted the fix-comments-in-repl branch October 28, 2015 02:03
thefourtheye added a commit that referenced this pull request Oct 28, 2015
repl: handle comments properly
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: #3421
PR-URL: #3515
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell commented Oct 28, 2015

Copy link
Copy Markdown
Member

Landed in v4.x-staging in d918838

jasnell mentioned this pull request Oct 28, 2015
Closed
rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
repl: handle comments properly
bd4311b 
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>
rvagg mentioned this pull request Oct 29, 2015
Merged
thefourtheye added a commit that referenced this pull request Oct 29, 2015
repl: handle comments properly
8a2c4ae 
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 mentioned this pull request Oct 29, 2015
Closed
This was referenced Apr 29, 2023
Open
Open
This was referenced Jun 22, 2023
Open
Open
Open
Open
Open
leonardoadame mentioned this pull request Jul 9, 2023
Open
aliscco mentioned this pull request Oct 27, 2023
Open
aliscco mentioned this pull request Nov 28, 2023
Open
This was referenced Nov 29, 2023
Open
Open
aliscco mentioned this pull request Dec 2, 2023
Open
leonardoadame mentioned this pull request Dec 3, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

repl Issues and PRs related to the REPL subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL