-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Changes from 3 commits
1953eab
30c680b
3e368ec
095d147
44688dd
a89f9fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tested and seen this behavior? Prior to this change, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What can we further improve FICS and IMHO, is whether:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 That will allow to call self.completer = (typeof options.completer === 'function')
? options.completer
: complete; 2- The next snippet, enforces the Interface.call(this, {
input: self.inputStream,
output: self.outputStream,
completer: self.completer,
terminal: options.terminal,
historySize: options.historySize,
prompt
}); 3- As long as the So, in conclusion, it seems that while 3 is enforced, meaning no one calls directly There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -698,6 +699,10 @@ function filteredOwnPropertyNames(obj) { | |
return Object.getOwnPropertyNames(obj).filter(intFilter); | ||
} | ||
|
||
REPLServer.prototype.complete = function() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are correct. My mistake! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
// | ||
|
@@ -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 | ||
|
@@ -967,7 +972,7 @@ REPLServer.prototype.complete = function(line, callback) { | |
|
||
callback(null, [completions || [], completeOn]); | ||
} | ||
}; | ||
} | ||
|
||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
|
@@ -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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
]); | ||
})); |
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.