Be willing to complete in the face of emptiness #772
+14
−0
Conversation
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
jennybc
force-pushed
the
bugfix/completing-nothing
branch
from
8790f19 to
cc1fb06
Compare
DavisVaughan
approved these changes
May 16, 2025
Comment on lines
+204
to
+216
| // Consider when the user asks for completions with no existing | ||
| // text-to-complete, such as at the R prompt in the Console, in an empty R | ||
| // file, or on an empty line of a non-empty R file. | ||
| // | ||
| // Gesture-wise, a Positron user could do this with Ctrl + Space, which | ||
| // invokes the command editor.action.triggerSuggest. | ||
| // | ||
| // The nominal completion node in these cases is just the root or 'Program' | ||
| // node of the AST. In this case, we should just provide "all" completions, | ||
| // for some reasonable definition of "all". | ||
| if x.node_type() == NodeType::Program { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I'm fine with the idea in general, but I don't think this belongs in is_identifier_like(), because the program node itself is not identifier-like.
I think at the call site it should be is_identifier_like(node) | is_empty_line(node) and is_empty_line() should contain this code.
The name is_empty_line is a WIP, if you have anything better to call this, feel free to use that.
There was a problem hiding this comment.
Yes, this is a definitely an improvement on the expressive-ness front. I'll do it.
jennybc
force-pushed
the
bugfix/completing-nothing
branch
from
cc1fb06 to
58f15d0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #770
Note that this layers on top of #805 which should be merged first. This PR currently targets that branch instead of
mainto make this clear.See the last "move" in #805 (comment) for a concrete illustration of what this PR does. If the client asks for completions in any context where we aren't inside a node (i.e. we're just in the 'Program' node), we should basically send all composite completions, instead of no completions.
I give this PR an A for correct behaviour and a C- for expressiveness. I really don't love that we're adding another special case to a free function named
is_identifier_like(). It feels likeis_identifier_like()is getting at some notion of "should we provide completions?" and this might be the time to articulate the intention more precisely. I suspect this property of the completion site could also become a peer ofparameter_hints_cellandpipe_root_cell, i.e. a feature we compute about the completion context early on and then use where needed.I would add a test before I merge this, but am holding off until after further discussion.