Breaking Change: make snippet-constructor more consistent

[L3MON4D3]

I'm thinking of moving around some of the arguments that can be passed to s.

The first time I thought about doing this was while adding snippetProxy (allows delayed (on expand) parsing of lsp-snippet), where the implementation could be a bit cleaner if the arguments to s were separated differently.
It has now come up again in #420, where it would be nice to specify condition/show_condition separately for each trigger (this obviously assumes that only the first table may be duplicated, nothing prevents also allowing multiple of the second table (apart from me believing it would be an ultimately worse desing and require a much more complicated implementation)).
I'm mentioning those two so there is some context for the suggested separation.

For a quick overview, we currently have:

s({
	trig, -- the trigger
	name, -- the snippets' name (displayed by eg. `cmp_luasnip`)
	wordTrig, -- only expand the snippet when the trigger begins on a word-boundary?
	regTrig, -- is the trigger a pattern?
	docstring, -- docstring for the snippet, in case we cannot generate it.
	docTrig, -- trigger used when generating the docstring.
	hidden, -- don't show the snippet in completion-engines?
	priority -- the snippet's priority.
}, nodes, {
	condition, -- potentially avoid snippet-expansion.
	show_condition, -- potentially avoid showing the snippet in completion-engines.
	callbacks, -- functions, called when something happens to some node inside the snippet.
	child_ext_ops, -- ext_opts for children.
	merge_child_ext_opts -- should the global ext_opts be merged into those of the snippet?
})

As of now, they are (rather subconciously) separated into options with "short" values (string, number, boolean) and those with "longer" values (functions, tables).
This obviously doesn't make very much sense and I'd like to change it.
(and here we also have a first problem: lots of configs will have to be adjusted when these parameters are rearranged. Is it worth it to cause that, without very much (slightly cleaner implementation for snippetProxy, more option in #420) benefit? Please let me know your opinions on this)

Currently, I'd favor a separation into options that somehow relate to expansion, and those that relate to the snippet after expansion (specifically this makes implementing snippetProxy nicer):

s({
	trig,
	wordTrig,
	regTrig,
	hidden, -- this is somewhat influenced by the problem stated in #420.
	priority, -- again, with #420 it could be useful to match one trigger earlier, and another later.
	condition,
	show_condition,
	name,
}, nodes, {
	docTrig, -- Not sure if docTrig should be here, it might be nice to have separate `docTrig`s for separate triggers.
	docstring,
	callbacks,
	child_ext_ops,
	merge_child_ext_opts
})

Another option, and this would prevent similar breaking changes in the future, would be to just put all parameters into one table.

What are your opinions? Objections? Questions?
Let me know :D

[leiserfg]

@L3MON4D3 to be honest I think is better to avoid doing this, even if the new API is nicer, it will break a lot of configurations, and to make it worse it will be probably a silent break.

[L3MON4D3]

Thank you👍
Yeah I'm very much on the fence with this.
It'd be nice to have, but we'd need to do a better job at switching to the new one than with add_snippets, and it wouldn't even matter that much after all.

[harrygallagher4]

It would be backwards-compatible to just merge both tables. That way you could break up the snippet metadata however you want. I guess it would be sort of odd, but I can't imagine a circumstance where it would cause issues unless in the future you wanted to reuse one of the "trigger" keys in the "post-expansion" data.

I've always found it sort of odd that there were two tables since both of them are just metadata applied to the snippet in the same context. As a fennel user I actually just wrote macros that take all of the metadata up front and then accept nodes as varargs.

[L3MON4D3]

That might be the best way forward 👍
I'll probably make condition and show_condition valid in the first table as well (also backwards compatible) so they are also per-trigger options for the multitrigger feature.

Yeah, the two tables exist because it felt right and I didn't spend much thought on it 😅

As a fennel user I actually just wrote macros that take all of the metadata up front and then accept nodes as varargs.

Oh nice 👌

[TimB87]

imo, from a users perspective, it might make sense to migrate the changes to a dev branch and maybe put out a warning on master so that people get time to migrate their snippets to the new syntax?
I know that not everybody would love to do that, but I feel like its important that you are able to do changes where they make sense, even if its a breaking some stuff.

[L3MON4D3]

Yup, I'm a bit hesitant to break probably every setup that uses lua-snippets for such small benefit though:
With ls.snippets -> add_snippets the transition was pretty rough, despite having a (okay, short :D) deprecation-period where both worked.
I guess the next time around I'll announce this stuff on reddit too, hopefully that'll reach more people.