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

VSCode now supports SnippetTextEdits natively #13229

Closed
Veykril opened this issue Sep 13, 2022 · 6 comments · Fixed by #16475
Closed

VSCode now supports SnippetTextEdits natively #13229

Veykril opened this issue Sep 13, 2022 · 6 comments · Fixed by #16475
Labels
A-vscode vscode plugin issues C-enhancement Category: enhancement

Comments

@Veykril
Copy link
Member

Veykril commented Sep 13, 2022

We should switch to that microsoft/vscode#145374 (comment), #13213 (comment)

@Veykril Veykril added the A-vscode vscode plugin issues label Sep 13, 2022
@jonas-schievink
Copy link
Contributor

I assume this has to wait until the October release is out, if we want to support N-1 versions of VS Code?

@Veykril
Copy link
Member Author

Veykril commented Sep 14, 2022

I suppose so? I don't really remember out stance in regards to required VSCode version, but waiting for october sounds reasonable

@lnicola
Copy link
Member

lnicola commented Sep 14, 2022

It's complicated. We've mostly supported more versions lately when someone at a FAANG asked for that. But they're using a fork of both Code and RA anyway, and I haven't heard much about their rebase schedule lately.

@Veykril Veykril added the C-enhancement Category: enhancement label Feb 13, 2023
@lnicola
Copy link
Member

lnicola commented Mar 13, 2023

I guess we could revisit this now? I skimmed the links, but it's not clear to me how well the new Code API matches our extension.

@davidbarsky
Copy link
Contributor

It's complicated. We've mostly supported more versions lately when someone at a FAANG asked for that. But they're using a fork of both Code and RA anyway, and I haven't heard much about their rebase schedule lately.

I'm decently sure that I'm the person in question. In any case, feel free to revisit this now: the sibling team responsible should rolling 1.75 in a few days. I might just ask that #14307 land before your changes land, it'd make the rebase/patching a bit easier on my end (I'm assuming #14307 will land this week).

@DropDemBits
Copy link
Contributor

I guess we could revisit this now? I skimmed the links, but it's not clear to me how well the new Code API matches our extension.

Unfortunately, the major difference is that the Code API (along with the related snippet APIs) performs whitespace fixups, and since we preserve the the original whitespace, this leads to extra indentation (see microsoft/language-server-protocol#724 (comment)). There also currently isn't a way to disable these fixups for SnippetTextEdits.

Thankfully, normal TextEdits insert the text as-is, so a workaround could be to isolate snippets to their own SnippetTextEdits and use TextEdits for everything else. This is definitely achievable with the structured snippet API (since it keeps track of where snippets need to be placed), though most assists don't use it yet. Finding where snippets are in TextEdits are is also an option, though that would require having to add a parser to find the exact locations of snippets.

A minor issue of this workaround is that this would technically go against the current specification of the snippetTextEdit extension (see #13213), although changing the definition of the spec seems to be preferred option (if I'm interpreting this comment right). Changing it also permits the user to immediately rename generated items by using multiple placeholder snippets in the same tabstop (which would be a way of solving sublimelsp/LSP-rust-analyzer#49).

In summary, we probably can't revisit this soon until we can work around the whitespace fixups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-vscode vscode plugin issues C-enhancement Category: enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants