-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
repl: assign underscore fix #3737
Conversation
@@ -545,6 +545,9 @@ REPLServer.prototype.createContext = function() { | |||
}); | |||
}); | |||
|
|||
// Refer: https://github.com/nodejs/node/pull/3729#issuecomment-155460861 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding here as well.
There was a problem hiding this comment.
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.
In the docs, where it says |
c5273ff
to
3d8bb15
Compare
@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 |
There was a problem hiding this comment.
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."
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: redefinitions
LGTM sans a few minor comments. |
@thefourtheye ... can you please rebase and update? |
63b27a6
to
71fe0a9
Compare
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.
71fe0a9
to
48d79a4
Compare
Sorry for the delay. I updated and rebased now. PTAL. |
LGTM |
Closing given that #5535 landed. |
As
_
is not defined in REPL's context, when it is defined asconst
,it breaks REPL, as it tries to store the result of the last evaluated
expression in
_
. This patch makes sure that_
is pre-defined inREPL'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
_
asconst
, also still assiging values to_
ispermitted.
Refer: #3729
Refer: #3704
cc @jasnell @cjihrig @targos @SamuelMarks @Fishrock123