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

repl: assign underscore fix by thefourtheye · Pull Request #3737 · nodejs/node · GitHub

/ node Public

repl: assign underscore fix - #3737

Closed
thefourtheye wants to merge 3 commits into
nodejs:masterfrom
thefourtheye:repl-assign-underscore-fix
ConversationCommits (3)ChecksFiles changed
Closed

repl: assign underscore fix#3737
thefourtheye wants to merge 3 commits into
nodejs:masterfrom
thefourtheye:repl-assign-underscore-fix

Conversation

thefourtheye commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

As _ is not defined in REPL's context, when it is defined as const,
it breaks REPL, as it tries to store the result of the last evaluated
expression in _. This patch makes sure that _ is pre-defined in
REPL's context, so that if users define it again, they will get error.

This patch has a test to make sure that the REPL doesn't allow
redefining _ as const, also still assiging values to _ is
permitted.

Refer: #3729
Refer: #3704

cc @jasnell @cjihrig @targos @SamuelMarks @Fishrock123

thefourtheye added the repl Issues and PRs related to the REPL subsystem. label Nov 10, 2015
cjihrig reviewed Nov 10, 2015
View reviewed changes
Comment thread lib/repl.js

cjihrig Nov 10, 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

Instead of referring to an issue, how about just explaining why we're doing this.

thefourtheye Nov 10, 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

Adding here as well.

jasnell Nov 12, 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

+1 to @cjihrig's comment. The reference to the issue is unnecessary, I think.

cjihrig commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

In the docs, where it says The special variable _ (underscore) contains the result of the last expression., maybe add another sentence saying that attempting to create a const _ variable will fail.

cjihrig mentioned this pull request Nov 10, 2015
Closed

thefourtheye commented Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

@cjihrig Did you mean to leave that comment in #3729?

thefourtheye force-pushed the repl-assign-underscore-fix branch from c5273ff to 3d8bb15 Compare November 10, 2015 16:43

thefourtheye commented Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

@cjihrig Updated as per the suggestions. PTAL.

cjihrig reviewed Nov 10, 2015
View reviewed changes
Comment thread doc/api/repl.markdown Outdated

cjihrig Nov 10, 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

Can you add "the" between "in" and "REPL."

cjihrig commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

A couple small comments, but LGTM. Once the CI is fully available, feel free to squash this and run the tests.

targos reviewed Nov 10, 2015
View reviewed changes
Comment thread lib/repl.js Outdated

targos Nov 10, 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

typo: redefinitions

jasnell commented Nov 12, 2015

Copy link
Copy Markdown
Member

LGTM sans a few minor comments.

jasnell commented Nov 13, 2015

Copy link
Copy Markdown
Member

@thefourtheye ... can you please rebase and update?

thefourtheye force-pushed the repl-assign-underscore-fix branch from 63b27a6 to 71fe0a9 Compare February 25, 2016 21:10
repl: pre-define _ in REPL's context
bcfa885 
As `_` is not defined in REPL's context, when it is defined as `const`,
it breaks REPL, as it tries to store the result of the last evaluated
expression in `_`. This patch makes sure that `_` is pre-defined in
REPL's context, so that if users define it again, they will get error.

Refer: nodejs#3729
Refer: nodejs#3704
test: make sure _ cannot be defined in REPL
c2a3731 
If the `_` is redefined as `const` in REPL, it will break the REPL, as
REPL will store the result of the last evaluated expression in `_`.
This patch has a test to make sure that the REPL doesn't allow
redefining `_` as `const`, also still assiging values to `_` is
permitted.

Refer: nodejs#3729
Refer: nodejs#3704
doc: warn about reassigning _ in REPL
48d79a4 
When users assign a value to `_` in REPL, it is prone to unexpected
results or messing up with REPL's internals. For example,
nodejs#3704. This patch issues a warning
about the same.
thefourtheye force-pushed the repl-assign-underscore-fix branch from 71fe0a9 to 48d79a4 Compare February 25, 2016 21:28

thefourtheye commented Feb 25, 2016

Copy link
Copy Markdown
Contributor Author

Sorry for the delay. I updated and rebased now. PTAL.

thefourtheye commented Feb 25, 2016

Copy link
Copy Markdown
Contributor Author

CI Run: https://ci.nodejs.org/job/node-test-pull-request/1754/

rvagg mentioned this pull request Feb 26, 2016
Closed
4 tasks

jasnell commented Mar 2, 2016

Copy link
Copy Markdown
Member

LGTM

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

Closing given that #5535 landed.

jasnell closed this Mar 22, 2016
thefourtheye deleted the repl-assign-underscore-fix branch March 22, 2016 06:09
This was referenced Mar 29, 2020
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
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.

5 participants

Back | FazBrowse Home | New Git URL