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

readline: enable tabs for input if tab completion is not enabled, fixes #1756 #1761

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions lib/readline.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,7 @@ function Interface(input, output, completer, terminal) {
}
historySize = historySize || kHistorySize;

completer = completer || function() { return []; };

if (typeof completer !== 'function') {
if (completer && typeof completer !== 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need completer &&?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no completer was explicitly provided, it will be undefined here. So valid values at this point are undefined (or more precisely, the way the code is, falsy values) or a function. It's only a TypeError if a completer was provided and it is not a function. So completer &&.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if it is a falsy value then its not a typeerror in this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my feeling. If you send it false (for example) then you are probably trying to say "Don't use a completer." In the spirit of being liberal with what is accepted but conservative with what is emitted, I wrote it up to allow falsy values. But strict checking for undefined would work too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait for others to take a look at this. I feel that we should allow only if it is a function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By allowing falsy values, we keep backwards compatibility. They were accepted before this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Okay then 👍

throw new TypeError('Argument \'completer\' must be a function');
}

Expand All @@ -72,9 +70,11 @@ function Interface(input, output, completer, terminal) {
this.historySize = historySize;

// Check arity, 2 - for async, 1 for sync
this.completer = completer.length === 2 ? completer : function(v, callback) {
callback(null, completer(v));
};
if (typeof completer === 'function') {
this.completer = completer.length === 2 ? completer : function(v, cb) {
cb(null, completer(v));
};
}

this.setPrompt('> ');

Expand Down Expand Up @@ -344,9 +344,6 @@ Interface.prototype._normalWrite = function(b) {
};

Interface.prototype._insertString = function(c) {
//BUG: Problem when adding tabs with following content.
// Perhaps the bug is in _refreshLine(). Not sure.
// A hack would be to insert spaces instead of literal '\t'.
if (this.cursor < this.line.length) {
var beg = this.line.slice(0, this.cursor);
var end = this.line.slice(this.cursor, this.line.length);
Expand Down Expand Up @@ -839,10 +836,6 @@ Interface.prototype._ttyWrite = function(s, key) {
this._deleteRight();
break;

case 'tab': // tab completion
this._tabComplete();
break;

case 'left':
this._moveCursor(-1);
break;
Expand All @@ -867,6 +860,14 @@ Interface.prototype._ttyWrite = function(s, key) {
this._historyNext();
break;

case 'tab':
// If tab completion enabled, do that...
if (typeof this.completer === 'function') {
this._tabComplete();
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

case 'tab'
  // If tab completion enabled, do that...
  if (typeof this.completer === 'function') {
    this._tabComplete();
    break;
  }

// else fall through to default
default:

May be better

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how I initially did it, but then make jslint complained:

Expected a "break" statement before "default" no-fallthrough

So I changed it to the arrangement in the PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a good solution is to extract the default block contents to its own function. The default block will run that function, of course, but so will the tab block if completer is undefined? So instead of falling through to default, the tab block would break like every other block, satisfying the linter.

On the other hand, the default block logic is not that complicated (yet, at least), and it is nice when reading a looooonnng switch/case like that to not have to change context to read the function invoked by default if it's short. So I could go either way on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you add a // falls through comment it will fix that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! Refactored per @Fishrock123 and added comment per @brendanashworth to let linter know that its intentional.

// falls through

default:
if (s instanceof Buffer)
s = s.toString('utf-8');
Expand Down
53 changes: 53 additions & 0 deletions test/parallel/test-readline-interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,59 @@ function isWarned(emitter) {
assert.equal(callCount, expectedLines.length);
rli.close();

// \t when there is no completer function should behave like an ordinary
// character
fi = new FakeInput();
rli = new readline.Interface({ input: fi, output: fi, terminal: true });
called = false;
rli.on('line', function(line) {
assert.equal(line, '\t');
assert.strictEqual(called, false);
called = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you assert.strictEqual(called, false) here and below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably a good idea to change all my instances of assert.ok() and assert.equal() to .strictEqual() since I know exactly what the value and type should be, so I'll go ahead and do that. Stop me if that's a terrible idea for some reason I'm not considering.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a terrible idea, just wouldn't really make a difference for most.

});
fi.emit('data', '\t');
fi.emit('data', '\n');
assert.ok(called);
rli.close();

// \t does not become part of the input when there is a completer function
fi = new FakeInput();
var completer = function(line) {
return [[], line];
};
rli = new readline.Interface({
input: fi,
output: fi,
terminal: true,
completer: completer
});
called = false;
rli.on('line', function(line) {
assert.equal(line, 'foo');
assert.strictEqual(called, false);
called = true;
});
fi.emit('data', '\tfo\to\t');
fi.emit('data', '\n');
assert.ok(called);
rli.close();

// constructor throws if completer is not a function or undefined
fi = new FakeInput();
assert.throws(function() {
readline.createInterface({
input: fi,
completer: 'string is not valid'
});
}, function(err) {
if (err instanceof TypeError) {
if (/Argument \'completer\' must be a function/.test(err)) {
return true;
}
}
return false;
});

// sending a multi-byte utf8 char over multiple writes
var buf = Buffer('☮', 'utf8');
fi = new FakeInput();
Expand Down