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

Add snippets via function only. #338

Closed
wants to merge 37 commits into from
Closed

Add snippets via function only. #338

wants to merge 37 commits into from

Conversation

L3MON4D3
Copy link
Owner

The goal of this PR is to switch from directly setting/getting snippets via ls.snippets to doing so via some function.

ls.snippets = {
	all = {
		...
	},
	lua = {
		...
	}
}

-- becomes

-- `type` is optional, may be "snippets" or "autosnippets".
ls.add_snippets("all", {...}, {type = "snippets"})
ls.add_snippets("lua", {...})
...
-- returns snippets, will be useful for eg. `cmp_luasnip`.
ls.get_snippets("all")

This is necessary to decouple the user-api from the internal structure of the snippet-storage (and should've been done a long time ago!), thereby enabling us to improve the internals (eg. in #334) without breaking changes in the future.
Adding snippets works fine using the functions, the on-demand-loading of snippets described in the wiki will not work any longer, and should be replaced by an (imo) autocommand-based implementation, similar to lazy_load in loaders.

After this is merged there will be a short deprecation-period (I'm thinking of two weeks, but feel free to suggest otherwise) during which existing direct access to the ls.snippet or ls.autosnippet-tables should be replaced by calls to add_snippets(ft, snippets, opts) or get_snippets(ft, opts).

As always, feel free to discuss these proposed changes.
If you have any workflows that rely on the current layout of the snippet-tables, please mention them here so we can find a solution.

@L3MON4D3 L3MON4D3 mentioned this pull request Mar 15, 2022
@leiserfg
Copy link
Contributor

@L3MON4D3 I think it will be good to tag the last commit before the change, so users can use the tag if they wanna delay moving to the new api.

@L3MON4D3
Copy link
Owner Author

Oh yes, that makes sense. Maybe a branch would also work? That could be created right now and kept up-to-date during the deprecation-period.

@jedrzejboczar
Copy link
Contributor

I'm currently removing duplicate snippets after all snippets have been loaded (both from_vscode and my own snippets). The code is something like ls.snippets[lang] = remove_list_duplicates(ls.snippets[lang], 'trigger'). Will there be a way to do this using the new API?

@leiserfg
Copy link
Contributor

Oh yes, that makes sense. Maybe a branch would also work? That could be created right now and kept up-to-date during the deprecation-period.

yes, that will work as well

@leiserfg
Copy link
Contributor

Anyway, I think adding a helper function that accepts a table (like the one we expose now) and calls add_snippets will make easier the migration.

@L3MON4D3
Copy link
Owner Author

I'm currently removing duplicate snippets after all snippets have been loaded (both from_vscode and my own snippets). The code is something like ls.snippets[lang] = remove_list_duplicates(ls.snippets[lang], 'trigger'). Will there be a way to do this using the new API?

Ah, right. I think the best way would be invalidating the snippet-objects, so they won't match and won't show up in lists, eg. cmp (Basically set hidden and prevent matching the snippet during expand). Then you could get all snippets and iterate over those, invalidating the ones that match.
Removing the snippet completely will be rather cumbersome I think, mainly because there will be multiple tables containing a reference to the snippet.

@L3MON4D3
Copy link
Owner Author

L3MON4D3 commented Mar 16, 2022

Anyway, I think adding a helper function that accepts a table (like the one we expose now) and calls add_snippets will make easier the migration.

Yess, that's true. I'm a tiny bit unsure about the changed semantics (current set vs. the when-merged add), but I don't think anyone relies on that.

@L3MON4D3
Copy link
Owner Author

Removing should work now.
You can get a table laid out like the current one (eg { ft1={...}, ft2={...} }) via ls.get_snippets(), iterating over that and calling snip:invalidate() on the ones you want to remove (followed by a ls.refresh_notify(ft) for all changed filetypes (maybe that is a bit impractical) for updating cmp_luasnip, if you use that) should do it.

@jedrzejboczar
Copy link
Contributor

This makes sense, thanks!

L3MON4D3 and others added 11 commits March 19, 2022 09:33
Makes more sense, config has to require some of the modules for the
default-config, while they themselves only need access to the config at
runtime.
Basically prevent circular dependencies.
before:
{
	snippets = {<snippets>},
	autosnippets = {<autosnippets>}
}

now:
{<snippets>}, {<autosnippets>}

As autosnippets are used much less than snippets, this should make
writing these files a bit more comfortable.
Bonus: it also keeps this format compatible with the one used
previously.
@L3MON4D3
Copy link
Owner Author

This can almost be merged, I'll add some improvements to invalidate, then we should be good to go.

@L3MON4D3
Copy link
Owner Author

Merged! But rebased, so closing this manually.

@L3MON4D3 L3MON4D3 closed this Mar 20, 2022
benfowler added a commit to benfowler/telescope-luasnip.nvim that referenced this pull request Apr 2, 2022
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.

3 participants