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.

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

Completion with text edits #602

Merged
merged 6 commits into from
Jun 7, 2019
Merged

Completion with text edits #602

merged 6 commits into from
Jun 7, 2019

Conversation

tomv564
Copy link
Contributor

@tomv564 tomv564 commented Jun 5, 2019

Sets up a post-completion hook so we can:

completionItem/resolve is used to get the additionalTextEdits if the server indicates support.

Code is tested using examples given in issues, but more live testing with specific servers is needed!
Assumptions:

  • If a command like 'commit_completion', 'insert_best_completion', 'auto_complete' runs, the next on_modified is the insertion of the completion.
  • It is always safe to trim the difference between the start of the current word and the textEdit's start.
  • All additionalTextEdits should be immediately applied after the completion is inserted.

To do:

  • Get feedback from clangd , metals and others?
  • Clean up code
  • Look for holes / add tests
  • UX considerations

@coveralls
Copy link

coveralls commented Jun 5, 2019

Coverage Status

Coverage decreased (-0.7%) to 38.224% when pulling 85af377 on completion-with-text-edits into 00a6330 on master.

@ayoub-benali
Copy link
Contributor

Thanks @tomv564 for taking a stab a this. I just made a test with Metals and both issues seems to be fixed 🎉

autocomplete

@olafurpg Do you have other use cases were this might still break ?

@olafurpg
Copy link

olafurpg commented Jun 6, 2019

This looks great! Nice job 👏

The only example I can think of is when textEdit spans multiple lines. This is prohibited by the spec and VS Code filters out such completions. In Metals we can still sometimes returns those, for example

// before
val name = "John"
val message = """
Hello $nam<COMPLETE>
"""
// after
val name = "John"
val message = s"""
Hello $name
"""

@ayoub-benali
Copy link
Contributor

@tomv564 Have you considered saving the additionalTextEdits contained in CompletionItem to apply them right after instead of doing a roundtrip via completionItem/resolve ?
LSP could be sending additionalTextEdits in the initial completion and not support completionItem/resolve

@tomv564
Copy link
Contributor Author

tomv564 commented Jun 6, 2019

Thanks for testing this so quickly, great to see your demo. Forgot to mention here, but @ayoub-benali your suggestions in #536 were key to getting this done!

In your example it's pretty clear that an import occurred, but imports added to the top won't be visible when you're two pages down. What do you think of showing a popup briefly (or less visibly, a message in the status bar)?

@ayoub-benali
Copy link
Contributor

In your example it's pretty clear that an import occurred, but imports added to the top won't be visible when you're two pages down. What do you think of showing a popup briefly (or less visibly, a message in the status bar)?

I think a status bar message is fine because in my example an import was added but in general we don't know which kind of text edits are sent by the server

@ayoub-benali
Copy link
Contributor

This looks great! Nice job

The only example I can think of is when textEdit spans multiple lines. This is prohibited by the spec and VS Code filters out such completions. In Metals we can still sometimes returns those, for example

// before
val name = "John"
val message = """
Hello $nam<COMPLETE>
"""
// after
val name = "John"
val message = s"""
Hello $name
"""

completion inside double quoted text isn't even triggered by sublime text see sublimehq/sublime_text#2793

Something has to be on sublime side before we can test this case

@tomv564
Copy link
Contributor Author

tomv564 commented Jun 6, 2019

@ayoub-benali LSP should already do what you suggested, the logic is roughly

if item.additionalTextEdits:
  apply_additional_edits(item.additionalTextEdits)
elif resolveProvider:
  resolve_completion_item(item)

@ayoub-benali
Copy link
Contributor

Awesome !

rwols
rwols previously approved these changes Jun 6, 2019
Copy link
Member

@rwols rwols left a comment

Choose a reason for hiding this comment

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

Seems to be working fine for clangd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants