repl: added support for custom completions#8484
repl: added support for custom completions#8484diosney wants to merge 1 commit intonodejs:masterfrom
Conversation
|
Simple example: var repl = require('repl')
.start({
terminal : true,
completer: function completer(line) {
var completions = 'aaa aa1 aa2 bbb ccc ddd eee'.split(' ');
var hits = completions.filter(function (item) {
return item.indexOf(line) == 0;
});
// Show all completions if none was found.
return [
hits.length
? hits
: completions,
line
];
}
}); |
|
Hi, any decision on this?. |
There was a problem hiding this comment.
Why not just pass line and callback by name?
There was a problem hiding this comment.
Just to avoid in the future if anyone adds more parameters to the original function but forgot to put them there. A little silly but removes complexity by having only one source of truth.
|
It's not up to me, but I'm OK with this change (assuming it works). |
|
OK, thanks for your opinion. I added the related tests too, so test if it works can be done automatically. |
|
Hi, any plans to review this? Thanks |
|
Also not up to me to accept it or not but this looks like a good addition. |
|
A few comments, but mostly LGTM. Please squash this down to a single commit. If no one has a problem with this, I'll land it. |
266bd4f to
231d34d
Compare
|
@cjihrig Sorry for the delay, somehow I got noticed right now of your comments about this :( I don't know why was until now that my email client delayed the related github notifications o_O. Thanks for your comments, I fixed all the pointed issues and squashed the commits into a single one, so, do you think that this feature can land now? Thanks |
There was a problem hiding this comment.
Are we forgetting to include a link here?
There was a problem hiding this comment.
Well, at the time I made the PR, that was the way in the other docs that the linking was done, maybe this changed in the meantime. Search in https://raw.githubusercontent.com/joyent/node/master/doc/api/https.markdown for [tls.createServer()][] for an example.
If there is other way to do it, just point me in the rigth direction please, but looking at the docs code I didn't found an example of direct linking.
There was a problem hiding this comment.
@thefourtheye Well, I ended direct linking to the page, I searched a bit more through the docs I found a way to do it. Compiled the docs and the links worked as expected.
There was a problem hiding this comment.
I think you need to use https://nodejs.org/api/readline.html#readline_readline_createinterface_options
There was a problem hiding this comment.
No, it seems that it had to be the relative path, something like readline.html#readline_readline_createinterface_options.
231d34d to
92d779e
Compare
|
@cjihrig OK, links in the docs fixed too, waiting for your approval. |
There was a problem hiding this comment.
Can you change this to self.completer = typeof options.completer === 'function' ? options.completer : complete; just to ensure that completer is a function.
Edit: You may have to split this across lines if it goes past 80 characters.
92d779e to
823dacb
Compare
|
@cjihrig Is everything okey now? I hope so :D |
There was a problem hiding this comment.
Do you not need to pass an error argument here?
There was a problem hiding this comment.
What do you mean? Can you be more explicit?
There was a problem hiding this comment.
Most callbacks take an error as their first argument. Is that not the case here?
|
Can you please open this PR against io.js. Since this is a new feature, it no longer makes sense to have it in joyent/node. |
|
LGTM, by the way. |
|
@cjihrig This is really frustrating man. Seriously. It seems that the Contributing docs have to be updated. |
|
Sorry, I'm not sure how closely you've been following along, but Node.js is moving into a foundation and merging with io.js. This repo (joyent/node) is going to become a maintenance branch. The new repo will live at nodejs/node. However, the converged repo is not quite ready yet, so new features (such as the one in this PR) are going into io.js. |
Currently I'm working on a project that uses the REPL to implement a mid complex CLI app found out that the REPL module doesn't have a way to override the default
complete()function, which is used for TAB completions of commands.I think that the REPL module can benefit from this addition, and specifically the repl applications that may wants to use this functionality.
I added the docs & tests, and the tests passed fine in my environment, this is just a little change, after all.