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

repl: added support for custom completions. #7527

Closed
Closed
Show file tree
Hide file tree
Changes from 4 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
3 changes: 3 additions & 0 deletions doc/api/repl.md
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 used for custom Tab 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
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(self);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the bind() necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tested and seen this behavior? Prior to this change, complete() was not bound. You also don't seem to need to bind any of the custom completers in your tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

complete() was not bound, but the inner self.complete was, which I think it makes complete.bind a must to not broken its implementation, since it relies on this.

What can we further improve FICS and IMHO, is whether:

  • custom options.completer needs to be binded too or not, since users could have access to useful REPL properties like this.lines, used in the default completer provided by the REPL.
  • the apply is deemed as not needed if the params are directly coded in REPL.prototype.complete, which will require then some logic to pass to the self.completer the correct parameters since it accepts one for sync operation and two for async operation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean. Could you add a test that fails when it is not bound? I just pulled down your changes, removed the bind(), and the entire test suite still passed.

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 You're right. I delve a little more and now I know exactly why it works without the extra binding:

1- In the following code snippet complete will set the correct this to self since it is defined as part of the object self, so it doesn't matter the implementation, but the object that defined it as part of one of its attributes.

That will allow to call REPL.completer always with the correct binding.

self.completer = (typeof options.completer === 'function')
    ? options.completer
    : complete;

2- The next snippet, enforces the this on Interface, so it does resets the internal this used by Interface.completer in the readline module, so it will always work: so no extra binding needed.

Interface.call(this, {
    input: self.inputStream,
    output: self.outputStream,
    completer: self.completer,
    terminal: options.terminal,
    historySize: options.historySize,
    prompt
  });

3- As long as the complete function is not called by directly, but through the REPL related method, the this will have the correct reference, and since all tests use replInstance.complete, then it works.

So, in conclusion, it seems that while 3 is enforced, meaning no one calls directly complete without setting the proper this, inside the repl file, it is not needed the extra binding :) so I will remove it and update the thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code updated.


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

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
67 changes: 66 additions & 1 deletion test/parallel/test-repl-tab-complete.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ testMe.complete('console.lo', common.mustCall(function(error, data) {
assert.deepStrictEqual(data, [['console.log'], 'console.lo']);
}));

// Tab Complete will return globaly scoped variables
// Tab Complete will return globally scoped variables
putIn.run(['};']);
testMe.complete('inner.o', common.mustCall(function(error, data) {
assert.deepStrictEqual(data, works);
Expand Down Expand Up @@ -283,3 +283,68 @@ if (typeof Intl === 'object') {
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'
]);
}));