repl: added support for custom completions

[diosney]

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.

[diosney]

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
      ];
    }
  });

[diosney]

Hi, any decision on this?.

[cjihrig]

Why not just pass line and callback by name?

[diosney]

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.

[cjihrig]

It's not up to me, but I'm OK with this change (assuming it works).

[diosney]

OK, thanks for your opinion. I added the related tests too, so test if it works can be done automatically.

[diosney]

Hi, any plans to review this? Thanks

[jasnell]

Also not up to me to accept it or not but this looks like a good addition.

[cjihrig]

Please make this ===

[diosney]

Fixed. Thanks.

[cjihrig]

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.

[diosney]

@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

[thefourtheye]

Are we forgetting to include a link here?

[diosney]

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.

[diosney]

@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.

[thefourtheye]

I think you need to use https://nodejs.org/api/readline.html#readline_readline_createinterface_options

[diosney]

No, it seems that it had to be the relative path, something like readline.html#readline_readline_createinterface_options.

[diosney]

@cjihrig OK, links in the docs fixed too, waiting for your approval.

[cjihrig]

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.

[diosney]

Done.

[diosney]

@cjihrig Is everything okey now? I hope so :D

[cjihrig]

Do you not need to pass an error argument here?

[diosney]

What do you mean? Can you be more explicit?

[cjihrig]

Most callbacks take an error as their first argument. Is that not the case here?

[cjihrig]

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.

[cjihrig]

LGTM, by the way.

[diosney]

@cjihrig This is really frustrating man. Seriously. It seems that the Contributing docs have to be updated.

[cjihrig]

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.