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.

Sign up for GitHub

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: added support for custom completions. #7527

Closed
wants to merge 6 commits into from
Closed

repl: added support for custom completions. #7527

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1953eab
repl: added support for custom completions.
diosney Jul 3, 2016
30c680b
repl: Make linter happy.
diosney Jul 4, 2016
3e368ec
repl: Removed unneeded console.log call.
diosney Jul 4, 2016
095d147
repl: Improved doc on `completer` function.
diosney Jul 5, 2016
44688dd
repl: Changed `var`s for `const`s in new tests.
diosney Jul 6, 2016
a89f9fe
repl: Removed unneeded extra binding.
diosney Jul 6, 2016
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
3 changes: 3 additions & 0 deletions doc/api/repl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,8 @@ added: v0.1.91
`undefined`. Defaults to `false`.
* `writer` {Function} The function to invoke to format the output of each
command before writing to `output`. Defaults to [`util.inspect()`][].
* `completer` {Function} An optional function that will used for custom Tab
Copy link
Member

Choose a reason for hiding this comment

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

"will be used"
Nit: I would prefer an active tense usage here, such as An optional function that is used for custom tab auto completion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lance: Updated doc to fix your nit. Check it out now and let me know.

auto completion. See [`readline.InterfaceCompleter`][] for an example.
* `replMode` - A flag that specifies whether the default evaluator executes
all JavaScript commands in strict mode, default mode, or a hybrid mode
("magic" mode.) Acceptable values are:
Expand Down Expand Up @@ -526,3 +528,4 @@ see: https://gist.github.com/2053342
[`util.inspect()`]: util.html#util_util_inspect_object_options
[here]: util.html#util_custom_inspect_function_on_objects
[`readline.Interface`]: readline.html#readline_class_interface
[`readline.InterfaceCompleter`]: readline.html#readline_use_of_the_completer_function
17 changes: 11 additions & 6 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,14 +386,15 @@ function REPLServer(prompt,
self.bufferedCommand = '';
self.lines.level = [];

function complete(text, callback) {
self.complete(text, callback);
}
// Figure out which "complete" function to use.
self.completer = (typeof options.completer === 'function')
? options.completer
: complete.bind(this);
Copy link
Contributor

@Fishrock123 Fishrock123 Jul 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

why is this binding necessary? (It looks like the context handling is already done by the apply, below.)

Copy link
Contributor Author

@diosney diosney Jul 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@Fishrock123 It is because the completer function is passed immediately to the Interface.completer option, so if is removed, then the function will not have the correct binding when executed by the readline module.

The apply binding is for the case when the complete function is called directly through the REPL instance, such as the tests.

Is this still viable or maybe you can give me any idea on what change I need to make in order to be accepted?

However, I will change this to self for consistency, it seems that I missed that.


Interface.call(this, {
input: self.inputStream,
output: self.outputStream,
completer: complete,
completer: self.completer,
terminal: options.terminal,
historySize: options.historySize,
prompt
Expand Down Expand Up @@ -698,6 +699,10 @@ function filteredOwnPropertyNames(obj) {
return Object.getOwnPropertyNames(obj).filter(intFilter);
}

REPLServer.prototype.complete = function() {
Copy link
Member

Choose a reason for hiding this comment

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

The way I read the code, it seems this function was added to REPLServer.prototype in order to allow for simpler testing. The result is a a greater API surface area. Is there a need for this function to be available at the user level? If not, perhaps is there a way to accomplish the testing requirements without adding to the prototype.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lance I'm not sure about its origins or all its purposes (besides the one related to testing), since it was already there when I added the feature, I just updated it so nothing is broken.

Copy link
Member

Choose a reason for hiding this comment

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

You are correct. My mistake!

Copy link
Member

@lance lance Jul 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Not really related to your PR, @diosney, but just to follow up on my comment. I've noticed this in a number of places - methods attached to an object's prototype that are ostensibly public, but undocumented. In my view, this is not ideal and should be rectified by deciding what is intended to be public and what is not, and structuring the code and modifying the docs so they reflect the true intent.

@Trott we spoke briefly about issues similar to this (e.g. someObj._someSemiPrivateProperty) during onboarding. I scanned through the issues but could not find anything that addressed this as an overarching concern. If there is some place where this is being discussed, can you point me in the right direction? If not, would it be reasonable to start this discussion with an issue on nodejs/node?

Copy link
Member

Choose a reason for hiding this comment

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

@lance If you can't find a place where it's been discussed, opening an issue is totally appropriate in my opinion. Worst case scenario is someone points you to another repo or venue to have the conversation and closes the issue.

this.completer.apply(this, arguments);
};

// Provide a list of completions for the given leading text. This is
// given to the readline interface for handling tab completion.
//
Expand All @@ -708,7 +713,7 @@ function filteredOwnPropertyNames(obj) {
//
// Warning: This eval's code like "foo.bar.baz", so it will run property
// getter code.
REPLServer.prototype.complete = function(line, callback) {
function complete(line, callback) {
// There may be local variables to evaluate, try a nested REPL
if (this.bufferedCommand !== undefined && this.bufferedCommand.length) {
// Get a new array of inputed lines
Expand Down Expand Up @@ -967,7 +972,7 @@ REPLServer.prototype.complete = function(line, callback) {

callback(null, [completions || [], completeOn]);
}
};
}


/**
Expand Down
125 changes: 98 additions & 27 deletions test/parallel/test-repl-tab-complete.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

var common = require('../common');
var assert = require('assert');
var repl = require('repl');
var repl = require('repl');

function getNoResultsFunction() {
return common.mustCall((err, data) => {
Expand All @@ -11,12 +11,13 @@ function getNoResultsFunction() {
});
}

var works = [['inner.one'], 'inner.o'];
var works = [['inner.one'], 'inner.o'];
const putIn = new common.ArrayStream();
var testMe = repl.start('', putIn);
var testMe = repl.start('', putIn);

// Some errors are passed to the domain, but do not callback
testMe._domain.on('error', function(err) {
testMe._domain.on('error', function (err) {
console.log(err)
assert.ifError(err);
});

Expand All @@ -28,13 +29,13 @@ putIn.run([
]);
testMe.complete('inner.o', getNoResultsFunction());

testMe.complete('console.lo', common.mustCall(function(error, data) {
testMe.complete('console.lo', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, [['console.log'], 'console.lo']);
}));

// Tab Complete will return globaly scoped variables
putIn.run(['};']);
testMe.complete('inner.o', common.mustCall(function(error, data) {
testMe.complete('inner.o', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, works);
}));

Expand All @@ -55,7 +56,7 @@ putIn.run([
'var top = function() {',
'var inner = {one:1};'
]);
testMe.complete('inner.o', common.mustCall(function(error, data) {
testMe.complete('inner.o', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, works);
}));

Expand All @@ -73,7 +74,7 @@ putIn.run([
' one:1',
'};'
]);
testMe.complete('inner.o', common.mustCall(function(error, data) {
testMe.complete('inner.o', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, works);
}));

Expand All @@ -87,7 +88,7 @@ putIn.run([
' one:1',
'};'
]);
testMe.complete('inner.o', common.mustCall(function(error, data) {
testMe.complete('inner.o', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, works);
}));

Expand All @@ -102,7 +103,7 @@ putIn.run([
' one:1',
'};'
]);
testMe.complete('inner.o', common.mustCall(function(error, data) {
testMe.complete('inner.o', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, works);
}));

Expand Down Expand Up @@ -152,47 +153,48 @@ putIn.run(['.clear']);
putIn.run([
'var str = "test";'
]);
testMe.complete('str.len', common.mustCall(function(error, data) {
testMe.complete('str.len', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, [['str.length'], 'str.len']);
}));

putIn.run(['.clear']);

// tab completion should not break on spaces
var spaceTimeout = setTimeout(function() {
var spaceTimeout = setTimeout(function () {
throw new Error('timeout');
}, 1000);

testMe.complete(' ', common.mustCall(function(error, data) {
testMe.complete(' ', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, [[], undefined]);
clearTimeout(spaceTimeout);
}));

// tab completion should pick up the global "toString" object, and
// any other properties up the "global" object's prototype chain
testMe.complete('toSt', common.mustCall(function(error, data) {
testMe.complete('toSt', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, [['toString'], 'toSt']);
}));

// Tab complete provides built in libs for require()
putIn.run(['.clear']);

testMe.complete('require(\'', common.mustCall(function(error, data) {
testMe.complete('require(\'', common.mustCall(function (error, data) {
assert.strictEqual(error, null);
repl._builtinLibs.forEach(function(lib) {
repl._builtinLibs.forEach(function (lib) {
assert.notStrictEqual(data[0].indexOf(lib), -1, lib + ' not found');
});
}));

testMe.complete('require(\'n', common.mustCall(function(error, data) {
testMe.complete('require(\'n', common.mustCall(function (error, data) {
assert.strictEqual(error, null);
assert.strictEqual(data.length, 2);
assert.strictEqual(data[1], 'n');
assert.notStrictEqual(data[0].indexOf('net'), -1);
// It's possible to pick up non-core modules too
data[0].forEach(function(completion) {
if (completion)
data[0].forEach(function (completion) {
if (completion) {
assert(/^n/.test(completion));
}
});
}));

Expand All @@ -202,7 +204,7 @@ putIn.run(['.clear']);
putIn.run([
'var custom = "test";'
]);
testMe.complete('cus', common.mustCall(function(error, data) {
testMe.complete('cus', common.mustCall(function (error, data) {
assert.deepStrictEqual(data, [['custom'], 'cus']);
}));

Expand All @@ -214,15 +216,15 @@ putIn.run([
'var proxy = new Proxy({}, {ownKeys: () => { throw new Error(); }});'
]);

testMe.complete('proxy.', common.mustCall(function(error, data) {
testMe.complete('proxy.', common.mustCall(function (error, data) {
assert.strictEqual(error, null);
}));

// Make sure tab completion does not include integer members of an Array
putIn.run(['.clear']);

putIn.run(['var ary = [1,2,3];']);
testMe.complete('ary.', common.mustCall(function(error, data) {
testMe.complete('ary.', common.mustCall(function (error, data) {
assert.strictEqual(data[0].indexOf('ary.0'), -1);
assert.strictEqual(data[0].indexOf('ary.1'), -1);
assert.strictEqual(data[0].indexOf('ary.2'), -1);
Expand All @@ -232,7 +234,7 @@ testMe.complete('ary.', common.mustCall(function(error, data) {
putIn.run(['.clear']);
putIn.run(['var obj = {1:"a","1a":"b",a:"b"};']);

testMe.complete('obj.', common.mustCall(function(error, data) {
testMe.complete('obj.', common.mustCall(function (error, data) {
assert.strictEqual(data[0].indexOf('obj.1'), -1);
assert.strictEqual(data[0].indexOf('obj.1a'), -1);
assert.notStrictEqual(data[0].indexOf('obj.a'), -1);
Expand Down Expand Up @@ -269,17 +271,86 @@ testMe.complete('.b', common.mustCall((error, data) => {
}));

const testNonGlobal = repl.start({
input: putIn,
output: putIn,
input : putIn,
output : putIn,
useGlobal: false
});

const builtins = [['Infinity', '', 'Int16Array', 'Int32Array',
'Int8Array'], 'I'];
const builtins = [
[
'Infinity', '', 'Int16Array', 'Int32Array',
'Int8Array'
], 'I'
];

if (typeof Intl === 'object') {
builtins[0].push('Intl');
}
testNonGlobal.complete('I', common.mustCall((error, data) => {
assert.deepStrictEqual(data, builtins);
}));

// To test custom completer function.
// Sync mode.
const customCompletions = 'aaa aa1 aa2 bbb bb1 bb2 bb3 ccc ddd eee'.split(' ');
const testCustomCompleterSyncMode = repl.start({
prompt : '',
input : putIn,
output : putIn,
completer: function completerSyncMode(line) {
var hits = customCompletions.filter((c) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

const

return c.indexOf(line) === 0
});
// Show all completions if none found.
return [hits.length ? hits : customCompletions, line];
}
});

// On empty line should output all the custom completions
// without complete anything.
testCustomCompleterSyncMode.complete('', common.mustCall((error, data) => {
assert.deepStrictEqual(data, [
customCompletions,
''
]);
}));

// On `a` should output `aaa aa1 aa2` and complete until `aa`.
testCustomCompleterSyncMode.complete('a', common.mustCall((error, data) => {
assert.deepStrictEqual(data, [
'aaa aa1 aa2'.split(' '),
'a'
]);
}));

// To test custom completer function.
// Async mode.
const testCustomCompleterAsyncMode = repl.start({
prompt : '',
input : putIn,
output : putIn,
completer: function completerAsyncMode(line, callback) {
var hits = customCompletions.filter((c) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

const here please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig Sorry, old ES5 habits. Updated.

return c.indexOf(line) === 0
});
// Show all completions if none found.
callback(null, [hits.length ? hits : customCompletions, line]);
}
});

// On empty line should output all the custom completions
// without complete anything.
testCustomCompleterAsyncMode.complete('', common.mustCall((error, data) => {
assert.deepStrictEqual(data, [
customCompletions,
''
]);
}));

// On `a` should output `aaa aa1 aa2` and complete until `aa`.
testCustomCompleterAsyncMode.complete('a', common.mustCall((error, data) => {
assert.deepStrictEqual(data, [
'aaa aa1 aa2'.split(' '),
'a'
]);
}));