Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

repl: assign underscore fix #3737

Closed

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

@thefourtheye thefourtheye added the repl Issues and PRs related to the REPL subsystem. label Nov 10, 2015
@@ -545,6 +545,9 @@ REPLServer.prototype.createContext = function() {
});
});

// Refer: https://github.com/nodejs/node/pull/3729#issuecomment-155460861
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
Contributor Author

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

cjihrig commented Nov 10, 2015

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
Contributor Author

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

@thefourtheye
Copy link
Contributor Author

@cjihrig Updated as per the suggestions. PTAL.

@@ -219,6 +219,10 @@ The special variable `_` (underscore) contains the result of the last expression
> _ += 1
4

*NOTE*: Explicitly assigning a value to `_` in the REPL can produce unexpected
results. Also, attempting to create `_` as a `const` variable in REPL will
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

cjihrig commented Nov 10, 2015

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

// Refer: https://github.com/nodejs/node/pull/3729#issuecomment-155460861
// REPL stores the result of the last evaluated expression in `context`'s `_`.
// But, in REPL, if `_` is defined as a `const`, then it will break the REPL.
// So, we define `_` first, so that later redefintions will fail.
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

jasnell commented Nov 12, 2015

LGTM sans a few minor comments.

@jasnell
Copy link
Member

jasnell commented Nov 13, 2015

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

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

@thefourtheye
Copy link
Contributor Author

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

jasnell commented Mar 2, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Closing given that #5535 landed.

@jasnell jasnell closed this Mar 22, 2016
@thefourtheye thefourtheye deleted the repl-assign-underscore-fix branch March 22, 2016 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants