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

Already on GitHub? Sign in to your account

REPL cmd without proceeding . #2980

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants

seebees commented Mar 21, 2012

If you execute a REPL command without a . it will result in a ReferenceError, catch the error and try for a REPL command.

This is related to #2974, but I think it could be useful in it's own right.

Clearly, I need tests and what-not. But this gives people a chance to comment and tell me I'm crazy or not. I'll put the details of how I would use this in #2974.

@TooTallNate TooTallNate and 2 others commented on an outdated diff Mar 21, 2012

lib/repl.js
@@ -208,7 +208,18 @@ function REPLServer(prompt, stream, eval, useGlobal, ignoreUndefined) {
self.context,
'repl',
function(e, ret) {
- if (e && !isSyntaxError(e)) return finish(e);
+ if (e && !isSyntaxError(e)) {
+ var tmp = e && (e.stack || e.toString());
+ if (tmp.match(/^ReferenceError/)) {
+ var matches = evalCmd.match(/^([^\s]+)\s*(.*)$/);
+ var keyword = matches && ('.' + matches[1]);
@TooTallNate

TooTallNate Mar 21, 2012

This style of conditionally assigning variables is rather cryptic. We try to stay readable throughout core, so these would ideally change to if statements.

@seebees

seebees Mar 21, 2012

@TooTallNate Sure thing. 215 and 216 as well? And do you want me to update this while I'm about it?

@isaacs

isaacs Mar 22, 2012

Don't update existing code (unless it's as a new commit which does explicitly that.) But yes, moving forward, do try to make it clear.

@TooTallNate TooTallNate and 1 other commented on an outdated diff Mar 21, 2012

lib/repl.js
@@ -208,7 +208,18 @@ function REPLServer(prompt, stream, eval, useGlobal, ignoreUndefined) {
self.context,
'repl',
function(e, ret) {
- if (e && !isSyntaxError(e)) return finish(e);
+ if (e && !isSyntaxError(e)) {
+ var tmp = e && (e.stack || e.toString());
+ if (tmp.match(/^ReferenceError/)) {
+ var matches = evalCmd.match(/^([^\s]+)\s*(.*)$/);
+ var keyword = matches && ('.' + matches[1]);
+ var rest = matches && matches[2];
+ if (self.parseREPLKeyword(keyword, rest) === true) {
+ return;
+ }
+ }
+ return finish(e);
+ }
@TooTallNate

TooTallNate Mar 21, 2012

I do wonder why you are modifying this part of the logic rather than here: https://github.com/seebees/node-1/blob/13e47b299dfcd2685aadc7d6fffb8de79efc1f61/lib/repl.js#L166-178

Any reasoning behind that?

@seebees

seebees Mar 21, 2012

@TooTallNate

  • break is a reserved word so as a REPL command you will aways need to type .break
  • backwards compatibility. the proceeding . is a nice way to disambiguate a variable from a REPL command, this is just sugar
  • variable assignment and REPL commands must still play nicely. I would be sad if someone ran .load file and said file had a variable called load or something
  • I did/do not know the extent to which people are extending the REPL, so I wanted something that was a light touch.

What would be nice is to abstract both sections so they use the same logic, but... I was figuring I'd use comments. The more I change, the more tests I need to write ;)

seebees commented Mar 22, 2012

@TooTallNate I still need to talk to @indutny and @rlidwka, but I think this is ready for review. I changed the implementation very slightly. But I think my tests are covering everything. The main problem I see is if someone is using '=' to designate something in a custom REPL command.
e.g. .myCmd =special or if someone wrote something crazy like:

clear
  = 'my Value'

But I don't feel strongly about that because of this:

myObj
.clear
 = function() {}

@isaacs Do you want a pull for this in v0.6? or only master?

isaacs commented Mar 22, 2012

Getting a repl just right is really hard. Humans are tricky interfaces :)

This definitely cannot be done in v0.6. All this talk is for master only.

isaacs commented Mar 22, 2012

I'd recommend, try something out, let's play with it a little bit, see how it feels, then iterate that way. It doesn't have to be production-ready to make a pull req, just make a note if it's still exploratory.

seebees commented Mar 22, 2012

Those pesky humans... I'll come back to it in a few days then. I feel about 75% confident with what is here. It's all about the edge cases ;)

Really I need more data to make sure I've got everything. That and what edge cases are just to crazy to try and support (e.g. my comments above...)

@seebees seebees REPL cmd without proceeding .
If you execute a REPL command without a . it will result in a ReferenceError, catch the error and try for a REPL command.
3502ee5

Can one of the admins verify this patch?

Closing this due to inactivity. If this is in error, please feel free to re-open or comment!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment