Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

REPL cmd without proceeding . #2980

Closed
wants to merge 1 commit into from

5 participants

seebees isaacs Nodejs Jenkins Chris Dickinson Nathan Rajlich
seebees

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.

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]);
Nathan Rajlich Owner

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 added a note

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

Nathan Rajlich Owner

Yes please :)

isaacs Owner
isaacs added a note

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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);
+ }
Nathan Rajlich Owner

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 added a note

@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 ;)

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

@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
Owner

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
Owner

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

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
Nodejs Jenkins

Can one of the admins verify this patch?

Chris Dickinson

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
Commits on Apr 18, 2012
  1. seebees

    REPL cmd without proceeding .

    seebees authored
    If you execute a REPL command without a . it will result in a ReferenceError, catch the error and try for a REPL command.
This page is out of date. Refresh to see the latest.
Showing with 158 additions and 0 deletions.
  1. +19 −0 lib/repl.js
  2. +139 −0 test/simple/test-repl-command-without-period.js
19 lib/repl.js
View
@@ -250,6 +250,25 @@ function REPLServer(prompt, stream, eval, useGlobal, ignoreUndefined) {
self.context,
'repl',
function(e, ret) {
+
+ // If there is an error and no bufferedCommand, it could
+ // be that someone is executing a REPL command
+ // without the proceeding '.'
+ // Let's find out
+ if (e && !self.bufferedCommand) {
+ // Same regEx from above, but missing the '.'
+ var matches = evalCmd.match(/^([^\s]+)\s*(.*)\s*$/);
+ if (matches && matches[2].charAt(0) !== '=') {
+ // add the proceeding '.' back in for parseREPLKeyword
+ var keyword = '.' + matches[1];
+ var rest = matches[2];
+ if (self.parseREPLKeyword(keyword, rest) === true) {
+ // Command exists, my work here is done
+ return;
+ }
+ }
+ }
+
if (e && !isSyntaxError(e)) return finish(e);
if (typeof ret === 'function' &&
139 test/simple/test-repl-command-without-period.js
View
@@ -0,0 +1,139 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var util = require('util');
+
+var repl = require('repl');
+
+// A stream to push an array into a REPL
+function ArrayStream() {
+ this.run = function(data) {
+ var self = this;
+ data.forEach(function(line) {
+ self.emit('data', line);
+ });
+ }
+}
+util.inherits(ArrayStream, require('stream').Stream);
+ArrayStream.prototype.readable = true;
+ArrayStream.prototype.writable = true;
+ArrayStream.prototype.resume = function() {};
+ArrayStream.prototype.write = function(data) {
+ this.data += data;
+};
+ArrayStream.prototype.data = '';
+
+var putIn = new ArrayStream();
+var testMe = repl.start('', putIn);
+
+// a command to test
+testMe.defineCommand('test', {
+ help: 'Just Something to test',
+ action: function(arg) {
+ arg = arg || '';
+ this.outputStream.write('Test called with:' + arg + '\n');
+ this.displayPrompt();
+ }
+});
+
+// sending a command should work
+putIn.run(['.test']);
+assert.equal(putIn.data, 'Test called with:\n');
+
+// reset condition
+putIn.data = '';
+
+// sending an argument to a REPL command should work
+putIn.run(['.test me']);
+assert.equal(putIn.data, 'Test called with:me\n');
+
+// reset condition
+putIn.data = '';
+
+// break is a reserved word, so sending it witout
+// a '.' should not execute the command but make
+// the REPL await another line
+putIn.run(['swtich(\'asdf\''
+ , 'case \'asdf\':'
+ , 'console.log(\'work\''
+ , 'break'
+ , '}']);
+assert.equal(putIn.data, '... ... ..... ..... ... ');
+
+// reset condition
+putIn.data = '';
+
+// on the other hand .break _is_ a REPL command,
+// so sending '.break' should execute the command
+putIn.run(['swtich(\'asdf\''
+ , 'case \'asdf\':'
+ , 'console.log(\'work\''
+ , '.break']);
+assert.equal(putIn.data, '..... ..... ....... ');
+
+// reset condition
+putIn.data = '';
+
+// sending a command witout a '.'
+putIn.run(['test']);
+assert.equal(putIn.data, 'Test called with:\n');
+
+// reset condition
+putIn.data = '';
+
+// sending an argument to a REPL command should work
+// even if we remove the '.'
+putIn.run(['test me']);
+assert.equal(putIn.data, 'Test called with:me\n');
+
+// reset condition
+putIn.data = '';
+
+// If a variable has the same name as a REPL
+// command, the REPL should prefer the
+// variable to the command
+putIn.run(['test = 5', 'test']);
+assert.equal(putIn.data, '5\n5\n');
+
+// reset condition
+putIn.data = '';
+
+// If I now clear the variable
+// the command should be revealed.
+// This test must follow the above test
+putIn.run(['clear', 'test']);
+assert.equal(putIn.data, 'Clearing context...\nTest called with:\n');
+
+// reset condition
+putIn.data = '';
+
+// The REPL should still return a
+// ReferanceError for something that
+// is not a variable nor a REPL command
+putIn.run(['noone']);
+// To avoid haveing to fix this test every time
+// a file in the stack trace changes, I split on \n
+// and look at the first line only
+var tmp = putIn.data.split('\n');
+assert.equal(tmp[0], 'ReferenceError: noone is not defined');
+
Something went wrong with that request. Please try again.