Skip to content

Conversation

@thefourtheye
Copy link
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

@thefourtheyethefourtheye added the repl Issues and PRs related to the REPL subsystem. label Nov 10, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Adding here as well.

Copy link
Member

Choose a reason for hiding this comment

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

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

@cjihrig
Copy link
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.

@thefourtheye
Copy link
ContributorAuthor

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

@thefourtheyethefourtheyeforce-pushed the repl-assign-underscore-fix branch from c5273ff to 3d8bb15CompareNovember 10, 2015 16:43
@thefourtheye
Copy link
ContributorAuthor

@cjihrig Updated as per the suggestions. PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@cjihrig
Copy link
Contributor

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

lib/repl.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

typo: redefinitions

@jasnell
Copy link
Member

LGTM sans a few minor comments.

@jasnell
Copy link
Member

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

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
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
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.
@thefourtheyethefourtheyeforce-pushed the repl-assign-underscore-fix branch from 71fe0a9 to 48d79a4CompareFebruary 25, 2016 21:28
@thefourtheye
Copy link
ContributorAuthor

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

@thefourtheye
Copy link
ContributorAuthor

@rvaggrvagg mentioned this pull request Feb 26, 2016
4 tasks
@jasnell
Copy link
Member

LGTM

@jasnell
Copy link
Member

Closing given that #5535 landed.

@jasnelljasnell closed this Mar 22, 2016
@thefourtheyethefourtheye deleted the repl-assign-underscore-fix branch March 22, 2016 06:09
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

replIssues and PRs related to the REPL subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@thefourtheye@cjihrig@jasnell@targos@MylesBorins