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

feat: snippet completions for functions #116

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

llllvvuu
Copy link
Contributor

@llllvvuu llllvvuu commented Jul 23, 2023

resolves #56

demo.mp4

tsserver does not create snippets for functions in the completions list; only upon completion resolve (presumably this is for performance reasons).

typescript-language-server deals with this by manually constructing the snippet and tweaking the inputTextFormat. We copy that here - as long as includeCompletionsWithSnippetText = true.

I chose to do one thing different fromtypescript-language-server: even though we can't put preview all the snippet params in the cmp menu, I still at least add "(...)" so that it visually distinguishes them as snippets. This is especially since nvim_cmp is unable to detect them as snippets.

Note that VSCode's own completeFunctionCalls feature is very rough around the edges. For example, it doesn't work on arrow functions, which is fundamentally because tsserver labels then as variables and so it doesn't make sense for them to be inputTextFormat = Snippet.

INFO:
refactor/fix: move default configs to config.lua so they can be referenced outside of just text_document/did_open
test: test both the completeFunctionCalls = true case and completeFunctionCalls = false case.

@llllvvuu llllvvuu force-pushed the feat/function_completions branch 2 times, most recently from a5b3bbe to 7cae371 Compare July 23, 2023 10:26
@llllvvuu llllvvuu force-pushed the feat/function_completions branch 3 times, most recently from 4220706 to 37a5e99 Compare July 23, 2023 11:31
`tsserver` does not create snippets for functions in the completions
list; only upon completion resolve (presumably this is for performance
reasons).

`typescript-language-server` deals with this by manually
constructing the snippet and tweaking the `inputTextFormat`. We copy
that here - as long as `includeCompletionsWithSnippetText = true`.

INFO:
refactor/fix: move default configs to `config.lua` so they can be
referenced outside of just `text_document/did_open`
@llllvvuu llllvvuu force-pushed the feat/function_completions branch from 37a5e99 to 1de9bd5 Compare July 23, 2023 11:32
@pmizio
Copy link
Owner

pmizio commented Jul 24, 2023

Very nice! Thank you for contribution! I looked into code and it looks fine, but can you add type annotations for new functions and ad least one test which contain new snippet response for regression purposes?
In the meantime I test it in my setup.

Also adds type annotations to function snippet feature.
@llllvvuu llllvvuu force-pushed the feat/function_completions branch from be16443 to c3046cc Compare July 24, 2023 10:37
@llllvvuu
Copy link
Contributor Author

@pmizio Sure, no problem, just added. For the tests, this feature actually turned the existing test cases into snippets, so I instead added back test cases for non-snippets.

@pmizio
Copy link
Owner

pmizio commented Jul 24, 2023

@pmizio Sure, no problem, just added. For the tests, this feature actually turned the existing test cases into snippets, so I instead added back test cases for non-snippets.

I mean more to assert constructed snippet which you add to resolve response here

@llllvvuu
Copy link
Contributor Author

I mean more to assert constructed snippet which you add to resolve response here

The value is the same as item.insertText: https://github.com/pmizio/typescript-tools.nvim/pull/116/files#diff-bb318b020a15617a49a83e92a30945b0ee02d37c1a9129c2768ade494dd91e03R105-R108

Which is tested here:

https://github.com/pmizio/typescript-tools.nvim/pull/116/files#diff-583bd8b57593fbf434045965a180c48661e07c2abd34cbf8c88da15d14e05ad9R197

I'm actually not sure how to trigger the item.textEdit.newText route specifically. It comes from item.replacementSpan being defined, which doesn't seem to be happening in any case for me.

@pmizio
Copy link
Owner

pmizio commented Jul 24, 2023

I mean more to assert constructed snippet which you add to resolve response here

The value is the same as item.insertText: https://github.com/pmizio/typescript-tools.nvim/pull/116/files#diff-bb318b020a15617a49a83e92a30945b0ee02d37c1a9129c2768ade494dd91e03R105-R108

Which is tested here:

https://github.com/pmizio/typescript-tools.nvim/pull/116/files#diff-583bd8b57593fbf434045965a180c48661e07c2abd34cbf8c88da15d14e05ad9R197

I'm actually not sure how to trigger the item.textEdit.newText route specifically. It comes from item.replacementSpan being defined, which doesn't seem to be happening in any case for me.

Oh, I'm blind sorry :D ok I will work with this for one or two days and merge it if everything will be ok. Once again thanks for contrib!

@pmizio
Copy link
Owner

pmizio commented Jul 24, 2023

@llllvvuu Just noticed one thing - ~ char is added multiple times when I leave completion:

Nagranie.z.ekranu.2023-07-24.o.15.04.03.mov

Edit: I checked it and it look like cmp add ~ additionally after resolve phase of completion. So in resolve you probably need to remove tiling ~ or just check out how cmp do things and send correct response in 1st phase of request.

@llllvvuu llllvvuu changed the title feat: function snippets feat: typescript.suggest.completeFunctionCalls (off by default) Jul 25, 2023
@llllvvuu llllvvuu force-pushed the feat/function_completions branch 2 times, most recently from 8c6457c to d3bfb7d Compare July 25, 2023 06:02
@llllvvuu
Copy link
Contributor Author

@pmizio good catch, I didn't realize it was nvim-cmp thing, so I removed the code that adds ~ on our side. Instead I added indicator (...). Unfortunately nvim-cmp logic doesn't allow me to trigger the ~ indicator on their side, but I opened an issue for this: hrsh7th/nvim-cmp#1665

I changed one more thing, which is to turn off the feature by default, similar to VSCode. I updated the README.md to show how to turn it on.

@llllvvuu llllvvuu force-pushed the feat/function_completions branch from d3bfb7d to bce78e7 Compare July 25, 2023 06:04
feat: copy the VSCode config option
`typescript.suggest.completeFunctionCalls` and have it off by default
@llllvvuu llllvvuu force-pushed the feat/function_completions branch 2 times, most recently from 2e94ec4 to f21fb2f Compare July 25, 2023 09:12
@pmizio
Copy link
Owner

pmizio commented Jul 27, 2023

Ok, so I tested this and it looks fine for me to merge, @llllvvuu do you want to change something before merge or I can do that?

@llllvvuu
Copy link
Contributor Author

@pmizio All good on my end 👍

@pmizio pmizio changed the title feat: typescript.suggest.completeFunctionCalls (off by default) feat: snippet completions for functions Jul 27, 2023
@pmizio pmizio merged commit bdc5878 into pmizio:master Jul 27, 2023
@pmizio
Copy link
Owner

pmizio commented Jul 27, 2023

Once again thanks for contrib @llllvvuu!

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.

VS Code provides parentheses completion for javascript
2 participants