Skip to content
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.

Sign up for GitHub

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

Jump to bottom

repl: add completion preview #30907

Closed
wants to merge 12 commits into from
Closed

repl: add completion preview #30907

wants to merge 12 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 11, 2019 •
edited
Loading

This mainly adds the next step to our the newly added preview: (tab) completion preview. It is now also automatically inserted when moving the cursor right when being at the end of the current input instead of only when pushing tab.

See it in action:
https://asciinema.org/a/ePQx0GfCYQGdnQTzwlnSIyxbN

This also fixes at least two small issues:

  1. image
    This is caused by the nested REPL tab completion. This was not detected earlier, since it's a rare case and only fails for syntax errors and similar. It was important to prevent syntax errors in the tab completion, otherwise the auto completion would have triggered those frequently.
  2. It also fixes an issue with the former preview implementation where long lines could have caused similar REPL confusion.

Please have a look at the individual commits messages for further details.

// cc: @nodejs/repl

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@BridgeAR BridgeAR requested a review from targos December 11, 2019 20:07
@nodejs-github-bot nodejs-github-bot added readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. labels Dec 11, 2019
@BridgeAR BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 11, 2019
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@targos

This comment has been minimized.

Sign in to view
@BridgeAR

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
targos
targos approved these changes Dec 12, 2019
View reviewed changes
lib/internal/readline/utils.js Outdated Show resolved Hide resolved
lib/internal/repl/utils.js Outdated Show resolved Hide resolved
lib/internal/repl/utils.js Outdated Show resolved Hide resolved
lib/readline.js Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member Author

@targos I addressed your nits and added one more commit that improves the coverage significantly. This should cover almost all regular preview usage plus edge cases.

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Trott
Trott reviewed Dec 13, 2019
View reviewed changes
doc/api/repl.md
**Default:** `true`. Always `false` in case `terminal` is falsy.
* `preview` {boolean} Defines if the repl prints autocomplete and output
previews or not. **Default:** `true`. Always `false` in case `terminal` is
falsy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean the default is true if terminal is truthy and false if terminal is falsy? If that's the case then I think the text can be clarified a little, but that's a nit and not a blocking comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually not only about the default. If the terminal is false, the preview is always deactivated. I am happy for suggestions to better word this :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of this

Always `false` in case `terminal` is falsy.

...this?:

If `terminal` is falsy, then there are no previews and the value of `preview` has no effect.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2019
@BridgeAR
repl: remove dead code
19dac98 
The .scope command was used only in the old debugger. Since that's
not part of core anymore it's does not have any use. I tried to
replicate the expected behavior but it even results in just exiting
the repl immediately when using the completion similar to the removed
test case.
@BridgeAR
repl: simplify repl autocompletion
c65f3f1 
This simplifies calling `filteredOwnPropertyNames()`. The context
is not used in that function, so there's no need to call the function
as such.
@BridgeAR
repl: simplify code
ec47482 
This simplifies some repl code and removes a coe branch that is
unreachable.
@BridgeAR
readline: update ansi-regex
43ae689 
This updates the used regular expression to the latest version.
It includes a number of additional escape codes.
@targos targos removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v12.x labels Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
repl: remove dead code
5b6e514 
The .scope command was used only in the old debugger. Since that's
not part of core anymore it's does not have any use. I tried to
replicate the expected behavior but it even results in just exiting
the repl immediately when using the completion similar to the removed
test case.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
repl: simplify repl autocompletion
22cc8a0 
This simplifies calling `filteredOwnPropertyNames()`. The context
is not used in that function, so there's no need to call the function
as such.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
repl: simplify code
dcc72b7 
This simplifies some repl code and removes a code branch that is
unreachable.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
readline: update ansi-regex
ad8f6d9 
This updates the used regular expression to the latest version.
It includes a number of additional escape codes.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
repl,readline: refactor common code
9d9652f 
This renames some variables for clarity and moves the common substring
part into a shared file. One algorithm was more efficient than the
other but the functionality itself was identical.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
repl,readline: refactor for simplicity
8839377 
This just refactors code without changing the behavior. Especially
the REPL code is difficult to read and deeply indented. This reduces
the indentation to improve that.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
repl: improve completion
f2d4db3 
This improves the completion output by removing the nested special
handling. It never fully worked as expected and required a lot of
hacks to even keep it working halfway reliable. Our tests did not
cover syntax errors though and those can not be handled by this
implementation. Those break the layout and confuse the REPL.

Besides that the completion now also works in case the current line
has leading whitespace.

Also improve the error output in case the completion fails.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
repl: add completion preview
4f54257 
This improves the already existing preview functionality by also
checking for the input completion. In case there's only a single
completion, it will automatically be visible to the user in grey.
If colors are deactivated, it will be visible as comment.

This also changes some keys by automatically accepting the preview
by moving the cursor behind the current input end.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
repl: fix preview bug in case of long lines
e800888 
This addresses an issue that is caused by lines that exceed the
current window columns. That would cause the preview to confuse the
REPL. This is meant as hot fix. The preview should be able to handle
these cases appropriately as well later on.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@BridgeAR @targos
test: add multiple repl preview tests
aaf2cf2 
This improves the coverage for the preview feature signficantly.
Quite a few edge cases get testet here to prevent regressions.

PR-URL: nodejs#30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
repl: remove dead code
78dcdee 
The .scope command was used only in the old debugger. Since that's
not part of core anymore it's does not have any use. I tried to
replicate the expected behavior but it even results in just exiting
the repl immediately when using the completion similar to the removed
test case.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
repl: simplify repl autocompletion
9b893e1 
This simplifies calling `filteredOwnPropertyNames()`. The context
is not used in that function, so there's no need to call the function
as such.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
repl: simplify code
6188650 
This simplifies some repl code and removes a code branch that is
unreachable.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
readline: update ansi-regex
0bafe08 
This updates the used regular expression to the latest version.
It includes a number of additional escape codes.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
repl,readline: refactor common code
6eda28c 
This renames some variables for clarity and moves the common substring
part into a shared file. One algorithm was more efficient than the
other but the functionality itself was identical.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
repl,readline: refactor for simplicity
8be0031 
This just refactors code without changing the behavior. Especially
the REPL code is difficult to read and deeply indented. This reduces
the indentation to improve that.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
repl: improve completion
7131de5 
This improves the completion output by removing the nested special
handling. It never fully worked as expected and required a lot of
hacks to even keep it working halfway reliable. Our tests did not
cover syntax errors though and those can not be handled by this
implementation. Those break the layout and confuse the REPL.

Besides that the completion now also works in case the current line
has leading whitespace.

Also improve the error output in case the completion fails.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
repl: add completion preview
bf9ff16 
This improves the already existing preview functionality by also
checking for the input completion. In case there's only a single
completion, it will automatically be visible to the user in grey.
If colors are deactivated, it will be visible as comment.

This also changes some keys by automatically accepting the preview
by moving the cursor behind the current input end.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
repl: fix preview bug in case of long lines
47dfa22 
This addresses an issue that is caused by lines that exceed the
current window columns. That would cause the preview to confuse the
REPL. This is meant as hot fix. The preview should be able to handle
these cases appropriately as well later on.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 28, 2020
@BridgeAR @targos
test: add multiple repl preview tests
f1624bb 
This improves the coverage for the preview feature signficantly.
Quite a few edge cases get testet here to prevent regressions.

PR-URL: #30907
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@targos targos targos approved these changes

@antsmartian antsmartian Awaiting requested review from antsmartian

@lance lance Awaiting requested review from lance

Assignees
No one assigned
Labels
notable-change PRs with changes that should be highlighted in changelogs. readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BridgeAR @nodejs-github-bot @targos @Trott @addaleax