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

Fix REPL assignment #4559

Merged
merged 1 commit into from
Jun 7, 2017
Merged

Fix REPL assignment #4559

merged 1 commit into from
Jun 7, 2017

Conversation

xixixao
Copy link
Contributor

@xixixao xixixao commented Jun 4, 2017

Is no one ever using the REPL? The variable was never documented, but Node does this on its own, so you can access the last expression with both _ and __. No more annoying messages!!!

@GeoffreyBooth
Copy link
Collaborator

I don’t understand what’s going on here. See #4528, which you were a part of too.

We’re assigning to _ explicitly because Node does, in order to override it. Why would we assign to __?

@lydell
Copy link
Collaborator

lydell commented Jun 7, 2017

So … the Node.js REPL has this feature that _ always refers to the value of the last expression, kind of like the “Ans” feature on some calculators (if you’ve ever used one of those).

As mentioned in a comment in the code, we add _ = before the inputted CoffeeScript code to force it to be an expression. So a variable name had to be chosen there. I guess _ was chosen since the Node.js REPL would clobber that variable anyway. However, newer versions of Node.js added the warning message whenever _ was assigned to.

So this PR simply uses another name (__) for the force-expression-assignment.

Everything works as before, with the exception that __ will now always be clobbered and we finally get rid of that annoying message.

I just tried this PR and it seems to be working fine:

$ ./bin/coffee
coffee> 1 + 1
2
coffee> _
2
coffee> __
2
coffee> 

@GeoffreyBooth
Copy link
Collaborator

Okay, @lydell if you think it’s okay then I’m fine with it. What about tests?

@lydell
Copy link
Collaborator

lydell commented Jun 7, 2017

I’m not sure there very much to test? And cake test passes locally on this branch.

@GeoffreyBooth
Copy link
Collaborator

A test that __ does, in fact, pull the last value. Or maybe we don’t care about this enough to bother?

@lydell
Copy link
Collaborator

lydell commented Jun 7, 2017

If @xixixao feels like writing a test then go for it, but if not it shouldn’t hold up this PR. There was no test before either.

@xixixao
Copy link
Contributor Author

xixixao commented Jun 7, 2017

I'm not bothered.

@GeoffreyBooth GeoffreyBooth merged commit 48c7deb into jashkenas:master Jun 7, 2017
@nigon135
Copy link

I'm not bothered

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants