-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
readline: show completions only after 2nd TAB #7754
Conversation
Code LGTM if CI is OK. |
Thanks for the reminder. ;) CI: https://ci.nodejs.org/job/node-test-commit/4129/ (edit: It’s green.) |
LGTM |
|
||
{ |
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.
Do we need {
block }
here ? Seems unnecessary
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.
It’s just to keep the diff small, I don’t feel strongly about it.
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.
@princejwesley @addaleax I think using let
on the following 2 line could make the block start scoping there.
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.
@yorkie Sorry, I’m not entirely sure what you’re suggesting – would you like to see the block removed? let
vs var
shouldn’t matter (but I would change var
-> let
if the block is removed, yes)
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.
let vs var shouldn’t matter
I don't think so because here the brackets would create a scope with let
declaration but not for var
. For example:
{
var f = completions.filter(function(e) { if (e) return e; });
var prefix = commonPrefix(f);
}
// here we actually can access `f` and `prefix`, this is not what we expect by brackets.
// using `let` instead of `var` can make it expected result.
Actually my suggestion is not removing brackets here and changing var
-> let
.
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.
any performance difference between let
vs var
is measured in nanoseconds (approaching a microsecond in one really strange past case, but has been fixed), and either way shouldn't be a consideration for this PR.
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.
So I think here using let
with block would reduce the possibility to produce bugs, because the variables are isolated to top scope.
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.
To summarize, if we use var
then remove the block scope. If we keep the block scope then switch to let
. Personally I don't care either way. @addaleax Let's do one or the other, but I'll leave that up to you.
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.
I’ve removed the extra block scope here and const
’ed the variables, that should be okay (but if anyone wants something else very strongly, I’ll change that again. I don’t really care that much either).
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.
const
variables looks good to me, too :-)
LGTM As to semver-minor vs. semver-major, this change could conceivably break scripted uses of the readline module. I don't know if anyone actually does that (besides us, in our test suite) but I'd err on the side of caution. |
37229c6
to
8c35e25
Compare
This LGTM. One thing we may want to consider as a follow on for this is to put some kind of time-limit for the second |
Show `TAB` completion suggestions only after the user has pressed `TAB` twice in a row, so that the full list of suggestions doesn’t present a distraction. The first time a `TAB` key is pressed, only partial longest-common-prefix completion is performed. This moves the `readline` autocompletion a lot closer to what e.g. `bash` does. Fixes: nodejs#7665
8c35e25
to
dca1f27
Compare
} | ||
|
||
// If there is a common prefix to all matches, then apply that portion. | ||
const f = completions.filter(function(e) { if (e) return e; }); |
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.
@addaleax
we don't need to run this block of code when completions.length === 1
, right?
I mean, self._insertString(completions[0].slice(completeOn.length))
is sufficient.
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.
@princejwesley Yeah… I’m not sure if that’s worth it, at least for tab completion. ;) If you want, I can add a strings.length === 1
shortcut case to commonPrefix()
, that’s probably where most of the unnecessary work would happen otherwise?
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.
@addaleax Yes. Its good to add length check in commonPrefix()
.
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.
@princejwesley Done! :)
Still LGTM! |
Show `TAB` completion suggestions only after the user has pressed `TAB` twice in a row, so that the full list of suggestions doesn’t present a distraction. The first time a `TAB` key is pressed, only partial longest-common-prefix completion is performed. This moves the `readline` autocompletion a lot closer to what e.g. `bash` does. Fixes: #7665 PR-URL: #7754 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in 1a9e247 |
Checklist
make -j4 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
readline
Description of change
Show
TAB
completion suggestions only after the user has pressedTAB
twice in a row, so that the full list of suggestions doesn’t present a distraction. The first time aTAB
key is pressed, only partial longest-common-prefix completion is performed.This moves the
readline
autocompletion a lot closer to what e.g.bash
does.Fixes: #7665
This is arguably an exclusively human-facing part of the
readline
functionality, so thesemver-major
label is up for debate.