repl: add completion preview - #30907
Closed
BridgeAR wants to merge 12 commits into
Closed
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
targos
reviewed
Dec 11, 2019
Comment thread
lib/internal/repl/utils.js
Outdated
BridgeAR
force-pushed
the
repl-completion-preview
branch
from
3414ff7 to
8ff770e
Compare
December 11, 2019 22:10
This comment has been minimized.
This comment has been minimized.
targos
approved these changes
Dec 12, 2019
Comment thread
lib/internal/readline/utils.js
Outdated
Comment thread
lib/internal/repl/utils.js
Outdated
Comment thread
lib/internal/repl/utils.js
Outdated
Comment thread
lib/readline.js
Outdated
BridgeAR
force-pushed
the
repl-completion-preview
branch
from
8ff770e to
9e1fd58
Compare
December 13, 2019 03:10
Copy link
Copy Markdown
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. |
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
BridgeAR
force-pushed
the
repl-completion-preview
branch
from
0fb2bd5 to
8f1921e
Compare
December 13, 2019 05:15
This comment has been minimized.
This comment has been minimized.
Trott
reviewed
Dec 13, 2019
Comment thread
doc/api/repl.md
Outdated
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Copy link
Copy Markdown
Member
Author
There was a problem hiding this comment.
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 :)
Sorry, something went wrong.
Copy link
Copy Markdown
Member
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Trott
approved these changes
Dec 13, 2019
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.
This simplifies calling `filteredOwnPropertyNames()`. The context is not used in that function, so there's no need to call the function as such.
This simplifies some repl code and removes a coe branch that is unreachable.
This updates the used regular expression to the latest version. It includes a number of additional escape codes.
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
This simplifies some repl code and removes a code branch that is unreachable. PR-URL: nodejs#30907 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
to targos/node
that referenced
this pull request
Apr 25, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
This simplifies some repl code and removes a code branch that is unreachable. PR-URL: #30907 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
targos
pushed a commit
that referenced
this pull request
Apr 28, 2020
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 <targos@protonmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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.
Please have a look at the individual commits messages for further details.
// cc: @nodejs/repl
Checklist