Add key to autoexpand snippets instead of having two separate tables

[aikow]

I'm currently trying to replicate my latex snippets using lua snip that I've previously had in ultisnips. Ultisnips has the snippet options, where you can simply append an A after your snippet to have it automatically expand.

I've been using this for snippets like

snippet lr "left right parenthesis" i
\left( $1 \right)$0
endsnippet

snippet () "left right parenthesis" iA
\left( $1 \right)$0
endsnippet

In luasnip, when loading these from a lua file, I have to place them in completely different parts of the document, since they have to be returned in 2 separate tables, which feels kind of clunky. I could, since the snippets are just lua code, add a bunch of table appending, extending, and manipulation code, but I feel like it shouldn't be to hard to just add it as an option to the snippet node, although I haven't looked at the code behind autosnippets yet.

Thanks for the cool library :grin:.

[L3MON4D3]

I have to place them in completely different parts of the document, since they have to be returned in 2 separate tables, which feels kind of clunky

feel like it shouldn't be to hard to just add it as an option to the snippet node

Hmmm, that's true.

The reason they have to be defined in separate tables is just an artifact of the way autosnippets were added to luasnip before add_snippets, which was done through a separate table ls.autosnippets. But now, with add_snippets, this limitation is gone, and not having any restraints on where snippets are defined is good, so I guess this should be added :+1: It should be enough to add autosnippet as another option in the first table passed to the snippet, similar to regTrig, I feel like usage would be a tad nicer with a separate autosnippet-function, but that can easily be implemented on top of the option.

[aikow]

Yeah, that sounds exactly like what I had in mind :).

Maintaining backwards compatibility should be simple enough as well, if thats desirable, by just adding the autotrigger = true to the second table returned by the snippets file.

[L3MON4D3]

Right that would work... though I'm not sure the current approach needs to be removed, I don't think the implementation would need to be much more complicated to support both

[atticus-sullivan]

@L3MON4D3 Did you already start working on this? Right now I've got exams, but after that (takes a while, ~end of the month) I'd look into this, if you're fine with that. (I know lua well, but not the vim api, but I think this task should be doable after reading your code)

[L3MON4D3]

Oh, no I have not :sweat_smile: I'm pretty busy right now, and there's other luasnip-related stuff I wanna get to before this, so you're very welcome to get on this :D

If you have any questions feel free to ask, I'll help where I can :)

[atticus-sullivan]

Alright, happy to help ;) I'm gonna write when something comes up. Eventually I think implementing this is the easy part, setting up the environment (e.g. running the tests) is the hard part (but only needed to be done once, after that contributing becomes much easier ^^)

[L3MON4D3]

Setting tests up should (:grimacing: xD) be as straightforward as running make test, let me know if it's more involved than this, maybe I can make it easier.

But this is definitely true for most projects :+1:

[atticus-sullivan]

See #508 for setup troubles. But for starting to code and thinking this through, I don't need to run the tests.

So back to this issue. I see there are two places where such a change makes sense:

1. In lua/luasnip/init.lua -> function add_snippets() we could filter out snippets which have a key autotriggered set to true Advantage: Easy to implement + nice code Disadvantage: Additional loop over the snippets (in addition to the one in lua/luasnip/session/snippet_collection.lua -> function add_snippets()) and one or two additional tables
2. In lua/luasnip/session/snippet_collection.lua -> function add_snippets() decide directly before inserting the snippet whether its an autosnippet or not. Advantage: No additional loop and no additional tables Disadvantage: The code might get a bit ugly, since on each snippet<->autosnippet switch, the internal tables prio_snip_table and ft_table have to be updated Advantage: No additional memory and no additional loop is needed (only some lookups for switching the above mentioned tables)

I have no experience how performance critical this part of the codebase is (of course I can imagine, especially the startup of nvim shouldn't be delayed...). As some sort of "proof of concept" (and to test a few things), I implemented approach 1) in this branch (we can of course still follow approach 2).

Any opinions on this topic?

[L3MON4D3]

Oh, I'm in favor of adding this in snippet_collection, the add_snippets in init.lua is (in my mind :sweat_smile:) more for providing a slightly more ergonomic interface to snippet_collection (and to actually call refresh_notify, which is used by eg. cmp_luasnip to update its internal list of snippet-items :wink:) And I agree, as is, that would become even more ugly.

I think the cleanest way to do add_snippets would be to:

1. first loop: determine which priorities occur in the snippets for this ft, set snip.priority, (potentially) set snip.autotriggered
2. create required tables (for the priorities found in 1.)
3. second loop: actually add snippets

What do you think?

Concerning performance, you can use luajits builtin profiler and flamegraph to do before-after-comparisons. But, concerning startup at least, all of this can just be wrapped via vim.schedule/is lazy_loaded already, hopefully.

[atticus-sullivan]

Oh of course (maybe, write a comment then with what function serves what purpose to make it clear when someone reads the code base).

So you'd collect the priorities (for autotriggered/normal) in some sort of set and create the required tables afterwards? That's how I understand 2., or did you mean to create these tables in this first loop? (the latter seems to be more straight forward and I don't think there is much gained by creating two new sets to determine afterwards what tables to create)

So at the moment, I'd set ft_table and prio_snip_table up for autotrig/normal snippets, and later (in the loop) add the snippet to the corresponding table (still keeping all in one loop). But I also think I don't quite get what you're describing.

According to your comment, you want to make the table-checks outside, but I think that's not possible as they depend on the snippet priority (thus they need to happen inside the context where the snippet is known aka inside the loop).

[L3MON4D3]

Oh of course (maybe, write a comment then with what function serves what purpose to make it clear when someone reads the code base).

Yes, some parts are severly lacking when it comes to comments :sweat_smile:

So you'd collect the priorities (for autotriggered/normal) in some sort of set and create the required tables afterwards? That's how I understand 2., or did you mean to create these tables in this first loop? (the latter seems to be more straight forward and I don't think there is much gained by creating two new sets to determine afterwards what tables to create)

Creating sets seems a bit more clean to me since the ifs in the loop are avoided, but that's not all that important.

Here's what I had in mind:

function M.add_snippets(snippets, opts)
	for ft, ft_snippets in pairs(snippets) do
		local prios = {
			autosnippets = {},
			snippets = {}
		}
		local types = {
			autosnippets = false,
			snippets = false
		}

		for _, snip in ipairs(ft_snippets) do
			snip.priority = opts.override_priority
				or (snip.priority ~= -1 and snip.priority)
				or opts.default_priority
				or 1000

			-- prefer more specific option I guess.
			-- snip.autotriggered may not acutally default to nil, that will
			-- cause problems with snippetProxy.
			snip.autotriggered = snip.autotriggered ~= nil and snip.autotriggered or opts.type == "autosnippets"

			snip.id = current_id
			current_id = current_id + 1

			types[snip.autotriggered and "autosnippets" or "snippets"] = true
			prios[snip.autotriggered and "autosnippets" or "snippets"][snip.priority] = true
		end

		for _, typename in ipairs({"autosnippets", "snippets"}) do
			-- only create table if there are snippets for it.
			if types[typename] then
				if not by_ft[typename][ft] then
					by_ft[typename][ft] = {}
				end

				local prio_snippet_table = by_prio[typename]
				for prio, _ in pairs(prios[typename]) do
					if not prio_snippet_table[prio] then
						prio_snippet_table[prio] = {
							[ft] = {}
						}
					elseif not prio_snippet_table[prio][ft] then
						prio_snippet_table[prio][ft] = {}
					end
				end
			end
		end

		for _, snip in ipairs(ft_snippets) do
			table.insert(by_prio[snip.autotriggered and "autosnippets" or "snippets"][snip.priority][ft], snip)
			table.insert(by_ft[snip.autotriggered and "autosnippets" or "snippets"][ft], snip)
			by_id[snip.id] = snip
		end
	end

	if opts.key then
		if by_key[opts.key] then
			invalidate_snippets(by_key[opts.key])
		end
		by_key[opts.key] = snippets
	end
end

I think that's pretty readable because we have the three, in themselves closed, steps, and it's not too hard to see what's going on in each (at least I think so, as the one who wrote it :P Do you agree?) (ofc. a few more comments on what's going on are missing :] )

[atticus-sullivan]

Yes knowing the structure (all the by_... tables) in which the snippet has to be stored, I think it is readable.

snip.autotriggered may not acutally default to nil, that will cause problems with snippetProxy.

With this change, snip.autotriggered should always be true or false, never nil (the boolean refers only to the snippet setting which is shadowed by the setting in add_snippets(_, opt), at least in your implementation. Might be confusing when someone tries to add a S(autotrig=false) with add_anippets(autotrig=true), thus this has to be documented).

Hm, now after all you coded this (this wasn't the plan ^^). How are we gonna to proceed now? (not that much experience with contributing to open source) I write a lot here what has to be/should be done, but no worries, I can also code ^^ (just a bit uncertain with some design decisions if its not my code in the first place)

Disclaimer: No solution for this problem/part of the code (in my opinion) but maybe a nice thought in general Thought came to me using an idiom as described here might be worth it here too (moving the table creation part to lua's metamethod ``). The big disadvantage with this is that if tab.key ... can't be used to check if this key is unset (as it will always assign and return a new empty table).

[L3MON4D3]

Yes knowing the structure (all the by_... tables) in which the snippet has to be stored, I think it is readable.

Okay, nice :)

snip.autotriggered may not acutally default to nil, that will cause problems with snippetProxy.

With this change, snip.autotriggered should always be true or false, never nil (the boolean refers only to the snippet setting which is shadowed by the setting in add_snippets(_, opt), at least in your implementation. Might be confusing when someone tries to add a S(autotrig=false) with add_anippets(autotrig=true), thus this has to be documented).

I'm not sure if how I wrote it there is actually the best way, we could also handle it similar to priority, eg. default autotriggered to -1 (so we know when it wasn't set in the snippet-constructor), and add_snippet's type provides the default for snippets where autotriggered is unset.

Hm, now after all you coded this (this wasn't the plan ^^).

Sorry :sweat_smile: I mainly wanted to get across how I imagined it, but yeah, I should've stuck to pseudocode.

How are we gonna to proceed now? (not that much experience with contributing to open source) I write a lot here what has to be/should be done, but no worries, I can also code ^^ (just a bit uncertain with some design decisions if its not my code in the first place)

Well, the code is just the end-product, deciding what goes where is the hard part, and with that you definitely contributed :) Go ahead and change it, this is just to illustrate what I had in mind.

Disclaimer: No solution for this problem/part of the code (in my opinion) but maybe a nice thought in general Thought came to me using an idiom as described here might be worth it here too (moving the table creation part to lua's metamethod ``). The big disadvantage with this is that if tab.key ... can't be used to check if this key is unset (as it will always assign and return a new empty table).

Hmmm, that's interesting, looks like it would lead to a much cleaner add_snippets... Seems too good to be true actually, would you do benchmarks to make sure it doesn't perform much worse if you decide to go with it? (Also, I think rawget can still be used to check if some key is unset)

Oh, could you open a PR for this? That's a bit more comfortable when reviewing code

[atticus-sullivan]

I'm not sure if how I wrote it there is actually the best way, we could also handle it similar to priority, eg. default autotriggered to -1 (so we know when it wasn't set in the snippet-constructor), and add_snippet's type provides the default for snippets where autotriggered is unset.

Why not. But 3 values, -1 = unset, 1 = set to true, 0 = set to false should be enough (just an example mapping). I see no need to be able to check how the key was set (on snippet creation or due to the add function).

I mainly wanted to get across how I imagined it, but yeah, I should've stuck to pseudocode.

^^ No harm done, I just felt bad, as you said, you'd wanted to do other stuff first, and then I came up with various things ^^

Hmmm, that's interesting, looks like it would lead to a much cleaner add_snippets...

Well that's for sure (I think).

Seems too good to be true actually, would you do benchmarks to make sure it doesn't perform much worse if you decide to go with it?

Yes, but this will take time (or not if I want to distract myself from other work ^^).

(Also, I think rawget can still be used to check if some key is unset)

Oh didn't thought of that

Oh, could you open a PR for this? That's a bit more comfortable when reviewing code

Yes, I'll use the your code from above for now, and try some things with these autotables. (And it should be a draft PR of course)

The posts keep getting longer with more and more discussion ^^

[L3MON4D3]

Why not. But 3 values, -1 = unset, 1 = set to true, 0 = set to false should be enough (just an example mapping). I see no need to be able to check how the key was set (on snippet creation or due to the add function).

:+1:

^^ No harm done, I just felt bad, as you said, you'd wanted to do other stuff first, and then I came up with various things ^^

Oh, no worries about that :D

Yes, but this will take time (or not if I want to distract myself from other work ^^).

xD

Yes, I'll use the your code from above for now, and try some things with these autotables. (And it should be a draft PR of course)

:+1:

The posts keep getting longer with more and more discussion ^^

Yeeeah, that's good though :D Better to discuss before than after merging it :P