Skip to content
This repository was archived by the owner on Oct 13, 2021. It is now read-only.

Use snippets.nvim to handle LSP snippets#172

Merged
haorenW1025 merged 2 commits intonvim-lua:masterfrom
runiq:snippets.nvim+completion-nvim
Aug 29, 2020
Merged

Use snippets.nvim to handle LSP snippets#172
haorenW1025 merged 2 commits intonvim-lua:masterfrom
runiq:snippets.nvim+completion-nvim

Conversation

@runiq
Copy link
Copy Markdown
Contributor

@runiq runiq commented Aug 11, 2020

I think snippets.nvim supports pretty much all of the LSP snippets syntax.

@runiq runiq marked this pull request as draft August 17, 2020 10:06
@runiq
Copy link
Copy Markdown
Contributor Author

runiq commented Aug 17, 2020

Sorry for the noise, I forgot I already opened a PR for this branch and kept pushing WIP stuff to it. :( I have converted this to a draft PR for now.

@runiq runiq marked this pull request as ready for review August 17, 2020 12:08
@clason
Copy link
Copy Markdown

clason commented Aug 20, 2020

@runiq Would it make sense to wrap the LSP snippets in a match_indentation by default? (I can't think of a reason you wouldn't want that.) You should be able to use the code fragment in the wiki.

@runiq
Copy link
Copy Markdown
Contributor Author

runiq commented Aug 22, 2020

I would have expected the LSP to handle that itself – can you give me an example where the inserted snippet results in wrong indentation?

@clason
Copy link
Copy Markdown

clason commented Aug 22, 2020

I would not have expected that ;) If you say that this already works out of the box, all the better!

@runiq
Copy link
Copy Markdown
Contributor Author

runiq commented Aug 24, 2020

@clason @mfussenegger, regarding this: Actually, now that I reread the spec, I believe it might be prudent to add a match_indentation if the textDocument/completion response contains an insertText snippet instead of a textEdit snippet. The spec says that editors are allowed to mess with an insertText after it's been inserted, while textEdits are supposed to be applied as-is.

Thoughts?

@hrsh7th
Copy link
Copy Markdown
Contributor

hrsh7th commented Aug 24, 2020

I think VSCode does indentation in both cases.

VSCode always applies accepting completion as a snippet.
https://github.com/microsoft/vscode/blob/f74e473238aca7b79c08be761d99a0232838ca4c/src/vs/editor/contrib/suggest/suggestController.ts#L358

(If the item is not insertTextFormat != 'snippet, VSCode does escape text and then insert as snippet)

@tjdevries
Copy link
Copy Markdown
Member

I haven't looked super closely at this, but I think it might make sense to instead have a PR that allows for "subscribing" to the completion items or something similar, and then you could have a separate plugin (completion-snippets.nvim or similar) that adds this so it doesn't have to be in this plugin.

Does that seem reasonable? I could maybe do that at some point if @haorenW1025 is too busy these days.

@mfussenegger
Copy link
Copy Markdown

@clason @mfussenegger, regarding this: Actually, now that I reread the spec, I believe it might be prudent to add a match_indentation if the textDocument/completion response contains an insertText snippet instead of a textEdit snippet. The spec says that editors are allowed to mess with an insertText after it's been inserted, while textEdits are supposed to be applied as-is.

Thoughts?

I'm not sure. In my use-case it's eclipse.jdt.ls which uses textEdit and the snippets don't contain the right indentation by themselves, here an example where the code should be indented:

newText = "@Override\nprotected ClusterBlockException checkBlock(CloseTableRequest request, ClusterState state) {\n\t${0:// TODO Auto-generated method stub\n\treturn super.checkBlock(request, state);}\n}",
demo usage

(This is not using this plugin, but my own variant and it's currently still using UltiSnips)

asciinema-2fdd309924b97885asciicast

@tjdevries
Copy link
Copy Markdown
Member

I think completion for snippets.nvim and snippets.nvim as LSP snippet manager should probably be split into two PRs.

What do you think @runiq ?

@runiq
Copy link
Copy Markdown
Contributor Author

runiq commented Aug 27, 2020

Sure, will do.

Edit: I'll leave this PR as the "LSP integration" PR since all the discussion on that is in this PR anyways; the other one will be the "Support snippets.nvim's own snippets" PR.

@runiq runiq changed the title Add support for snippets.nvim Use snippets.nvim to handle LSP snippets Aug 27, 2020
@runiq
Copy link
Copy Markdown
Contributor Author

runiq commented Aug 27, 2020

@tjdevries The non-LSP stuff is now in #180.

@haorenW1025
Copy link
Copy Markdown
Collaborator

haorenW1025 commented Aug 28, 2020

Sorry for the long wait and thanks @runiq for this PR and others for keeping the discussion going. I'm quite busy for the past several weeks but I'm getting back into completion-nvim now:). @runiq to my understanding this PR now only contains the LSP handling now, is that correct? And is this ready for merge? If it is I'll try testing it these days.

@runiq
Copy link
Copy Markdown
Contributor Author

runiq commented Aug 28, 2020

Glad to have you back :)

Yes, exactly, only LSP handling. I think it's ready for review, at least.

Comment thread lua/completion.lua Outdated
@haorenW1025
Copy link
Copy Markdown
Collaborator

I've tested it and it seems good for me. However I kind of like the LSP snippets to work "out of the box" instead of setting vim.g.completion_enable_snippet. Before we only have vim-vsnip as a available method so it's easier but now we should refactor bunch of stuff if we want it that way... I'll merge it first and we can refactor that later on. Thanks again @runiq !

@haorenW1025 haorenW1025 merged commit ade764f into nvim-lua:master Aug 29, 2020
@clason
Copy link
Copy Markdown

clason commented Aug 29, 2020

However I kind of like the LSP snippets to work "out of the box" instead of setting vim.g.completion_enable_snippet. Before we only have vim-vsnip as a available method so it's easier but now we should refactor bunch of stuff if we want it that way...

I agree that the correct approach would be to automatically enable snippets (and set the corresponding client capability) if a supported snippet plugin is detected.

@runiq runiq deleted the snippets.nvim+completion-nvim branch August 29, 2020 13:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants