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: add snippet completion for injected languages #226

Merged
merged 5 commits into from
Dec 9, 2021

Conversation

ner0-m
Copy link

@ner0-m ner0-m commented Nov 25, 2021

This is the port of this PR for cmp_luasnip.

It uses the language at the cursor position by default and falls back to the filetype if treesitter isn't available. I'm not sure about the parameter if such a thing is good. I appreciate the feedback!

@leiserfg
Copy link
Contributor

IMHO it should not be the default, but configurable, it's true that tree-sitter is more precise in embedded languages, but there are some users of multi-filetype (like 'markdown,jekyll') and for them, it will be a breaking change.

@leiserfg
Copy link
Contributor

Maybe another option is to make it "smarter" and fallback to the vim.bo.filetype logic if tree-sitter says the type on the cursor position is part of vim.bo.filetype. For example, if editing a markdown,jekyll file filetypes should be:

  • some-x-lang when editing a code-fence of type some-x-lang as it's not part of the vim.bo.filetype.
  • {'mardown', 'jekyll'} otherwise.

That's maybe too convoluted but should make multi-filetypes work as expected.

@L3MON4D3
Copy link
Owner

We could provide a setting, ft_func, a function that would be called here and returns the list of filetypes that should be extended with ft_redirect.
Default would just be function() return vim.split(vim.bo.filetype, ".", true) end, we could collect other functions (treesitter, for now) in extras/filetype_functions or something like that. That would make it pretty extensible (maybe LSP will get/already has something similar?) and easy to override whatever default behaviour for merging ft and treesitter's filetype we choose.

(sidenote: we probably also need some mechanism for switching configs based on filetype, out of scope for this particular PR, but something to consider).

@L3MON4D3
Copy link
Owner

Also ty @ner0-m for porting it so quickly :)

@ner0-m
Copy link
Author

ner0-m commented Nov 26, 2021

Also ty @ner0-m for porting it so quickly :)

You're very welcome :)

I liked the idea of having a configurable function and that's the current status. It shouldn't be to hard to do something like @leiserfg mentioned, or any other behavior for that matter.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Nov 26, 2021

Nice, this looks very clean 👌

One thing I just realized: lazy_load will have to be adjusted to load the snippets for injected filetypes.
Currently they are loaded on events BufWinEnter,Filetype, if the cursor isn't on top of the injected filetype at that point we don't load them at all.
Right now I can think of checking for new lazy_load-filetypes in expand, before the snippets are searched , but that will probably make autosnippets (where expand has to be called on each keystroke) lag pretty hard if from_cursor_pos is used.
Alternatively we could provide a command that triggers a reload, but I'd rather it happen automatically.

The lazy_load-check probably won't affect autosnippets harder than from_cursor_pos, should it be used.
Any ideas?

@leiserfg
Copy link
Contributor

One option could be deprecate lazy loading, we have not measured how fast is the normal loader since we added the new JSON parser.

@leiserfg
Copy link
Contributor

Other way could be to trigger a custom event once a new ft is found by the new heuristic and subscribe the lazy load to it as well.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Nov 26, 2021

One option could be deprecate lazy loading, we have not measured how fast is the normal loader since we added the new JSON parser.

Tempting.. :D

Other way could be to trigger a custom event once a new ft is found by the new heuristic and subscribe the lazy load to it as well.

That sounds good, although a custom event is probably overkill, just calling from expand() should do the trick?
Also, correcting my previous statement, if from_cursor_pos is used treesitter will have to incrementally parse the buffer either way, so it won't hurt much more to check for the lazy_load-filetypes on expand.
Concerning the implementation, we could just make _luasnip_vscode_lazy_load accept a list of filetypes, that way they won't have to be computed twice per expansion.

@ner0-m
Copy link
Author

ner0-m commented Nov 28, 2021

Mhhh, I'm a little lost now. Can you help me out, with the changes to the lazy load? :)

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Nov 28, 2021

Actually, even if we load the new snippets in match_snippet, they won't be available until later because the files are read asynchronously, so this is probably something to tackle later.
For now, could you put a note in the doc, something like "If snippets are loaded from vscode-style packages, they will have to be loaded using require("luasnip.loaders.from_vscode").load() to be available if the from_cursor-function is used."?

@leiserfg
Copy link
Contributor

I said a new event 'cause the core does not know about loaders.

@L3MON4D3
Copy link
Owner

Also true, putting the snippets into the main module was a bit of a mistake to begin with 😅.
We could move the snippets to session, that would remove the dependency of loader on luasnip

@leiserfg
Copy link
Contributor

I was thinking about the option of having several sources of snippets as for example:

  1. the current snippets table
  2. the vscode snippets loader
  3. a future snipmate loader

Each one provides a way to ask for a list of snippets for certain ft

That will remove the issue of having to call the loaders after setting the snippet table and could make it possible to lazy load or not depending of the intricacies of each loadable format.

@L3MON4D3
Copy link
Owner

Not a bad idea, that way we could also reload snippets for a single source only 👍
That way we could also implement the hide-ability in luasnip as opposed to in each completion-provider (there's only cmp_luasnip now, but if there were more it'd be bad to have each implement these mechanism)

a future snipmate loader

👀

@smjonas
Copy link

smjonas commented Dec 9, 2021

@ner0-m First of all, this is an amazing idea! Would you mind if I use some of your treesitter-related code for cmp-nvim-ultisnips? I will of course credit you since this was your idea and initial implementation.

@ner0-m ner0-m force-pushed the feat/fs-for-language-injection branch from 0a45d4c to f813c0a Compare December 9, 2021 16:07
@ner0-m
Copy link
Author

ner0-m commented Dec 9, 2021

I've finally found time to get back to this, sorry for the delay, my studies keep me busy.

I've added a note in the docs regarding the lazy loading and make changed the formatting of the functions. Any other suggestions?

@smjonas I'd be very happy and don't mind it at all!

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Dec 9, 2021

No problem, and I've no more suggestions, very pleased with this PR 👌
Thanks a lot, another great feature for Luasnip :D

@L3MON4D3 L3MON4D3 merged commit a784097 into L3MON4D3:master Dec 9, 2021
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.

None yet

5 participants