repl: added support for custom completions. #7527
Conversation
|
This change looks like it introduces a number of whitespace-related lint errors. Try running |
| // Figure out which "complete" function to use. | ||
| self.completer = (typeof options.completer === 'function') | ||
| ? options.completer | ||
| : complete.bind(this); |
There was a problem hiding this comment.
why is this binding necessary? (It looks like the context handling is already done by the apply, below.)
There was a problem hiding this comment.
@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.
|
@Trott You are right, it seems that my IDE added those on Maybe we can add the |
|
@Trott @Fishrock123 I added a commit with the code correctly linted, can you check it out and let me know about other changes? Thanks! Do I have to squash commits into one? Tell me if so. |
|
Any ideas on why the those tests are failing? Since my dev partition is in |
|
I strongly suspect NFS will mess up some of the |
|
NFS != NTFS, this seems more like permission mixups, and that is probably outside of Node’s scope… only the |
|
FWIW, I checked out your branch and ran the tests with no failures. |
| @@ -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 | |||
There was a problem hiding this comment.
"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.
@lance: Updated doc to fix your nit. Check it out now and let me know.
jasnell
added
the
semver-minor
Nice! Thanks for running the tests for me! |
| // 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.
Is the bind() necessary here?
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.completerneeds to be binded too or not, since users could have access to useful REPL properties likethis.lines, used in the default completer provided by the REPL. - the
applyis deemed as not needed if the params are directly coded inREPL.prototype.complete, which will require then some logic to pass to theself.completerthe correct parameters since it accepts one for sync operation and two for async operation.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
The FreeBSD failure on CI is an unrelated flaky test that will be fixed in #7555. |
|
LGTM |
2 similar comments
|
LGTM |
|
LGTM |
|
Landed in 9fbe456 |
Allow user code to override the default `complete()` function from `readline.Interface`. See: https://nodejs.org/api/readline.html#readline_use_of_the_completer_function Ref: nodejs/node-v0.x-archive#8484 PR-URL: #7527 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com>
Allow user code to override the default `complete()` function from `readline.Interface`. See: https://nodejs.org/api/readline.html#readline_use_of_the_completer_function Ref: nodejs/node-v0.x-archive#8484 PR-URL: #7527 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Lance Ball <lball@redhat.com>
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesIt threw errors but not related to the new changes for what I can say.
I ran
./node ./test/parallel/./test/parallel/test-repl-tab-complete.jsand it does it OK. At the end is the error log JIC.UPDATE:
It seems that is an issue with my dev environment, @lance (see in commets) ran the tests and they ran OK.
Affected core subsystem(s)
repl, doc
Description of change
Original description of PR, since this is an old PR of mine to the extint v0.13.x, references of old PR at the end.
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 readline
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.
Simple usage example:
References for the old PR:
nodejs/node-v0.x-archive#8484
Error thrown by
make -j4 test: