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

Multi-context-snippets #774

Merged
merged 7 commits into from
Mar 8, 2023
Merged

Multi-context-snippets #774

merged 7 commits into from
Mar 8, 2023

Conversation

L3MON4D3
Copy link
Owner

Example:

ls.setup_snip_env()

local ms = require("luasnip.extras.multi_snippet").new_multisnippet

ls.add_snippets("all", {
	ms({
		{trig = "a", condition = function(ltc) return ltc ~= "a" end, snippetType = "autosnippet"},
		{trig = "b", condition = function(ltc) return ltc ~= "b" end},
		"c"
	}, {t"a or b"})
}, {
	key = "ab"
})
1676497115.mp4

The main changes/ideas to accomodate this are:

  • anything inside a table passed to add_snippets is expected to have a function, retrieve_all which returns all "expandable" objects associated with that "addable"

  • "expandable" objects only have to be able to perform

    • matches (for match_snippet in snippet_collection)
    • copy (currently copies the snippet stored in snippet_collection, since we need each of the snippets inserted into the buffer to be unique)
    • get_docstring (to get docstring shown in completion-engines) and
    • invalidate (to disable the snippet, but not yet delete it from the collection)

    but don't have to be actual snippets, which allows us to point multiple of these "expandables" to just one snippet-object (it just has to be returned on copy)

Missing:

  • Allow opts (at all, and different ones (maybe only different conditions)). If we allow

Stuff to think about:

  • current api of ms is not that great, we'd probably want to do some merging of the tables, such that common keys don't have to be repeated. Not sure what's the user-friendliest behaviour here
  • I also allowed context, the first table passed to s, to contain condition and show_condition. I'd like to make that the default shown in the docs since it makes more sense (conceptually: the first table is for stuff affecting the expansion of the snippet, while the third/[second non-nodes-] table is for stuff affecting the expanded snippet), while still respecting the conditions when set in the opts-table. I'm uncertain whether here, in this new, so-far-unused function, we should also accomodate the conditions when they're set in the opts-table. It would make more sense without knowing too much about the internals ("why does this behave differently from s??"), but would slightly complicate the implementation. Seems better to allow it, but worth thinking about.

Finally:
I think I'll add e88165f not in this PR, but directly to master, since it is completely tangential and I just noticed it while working on this.

@L3MON4D3
Copy link
Owner Author

Some more changes:
keys common to all contexts can be set in the common-key in the table where
the differen contexts reside.
I believe this is better than

  • having contexts[1] or contexts[#contexts] be the common table, since this
    makes the common table required
  • merging the tables, such that keys in contexts[2] would be extended by those
    in contexts[1], since this seems like it would introduce more cognitive
    overhead to use, and would probably include lots of resetting keys to their
    defaults.

An example:

ls.setup_snip_env()

local ms = require("luasnip.extras.multi_snippet").new_multisnippet

local first_a = require("luasnip.extras.conditions").make_condition(function(ltc)
	return ltc:sub(1,1) == "a"
end)

ls.add_snippets("all", {
	ms({
		-- this is applied to all of the multiple contexts.
		common = {trig = "a"},
		-- first character not a => autotrigger
		{condition = -first_a, snippetType = "autosnippet"},
		-- otherwise: still allow manual triggering
		{condition = first_a},
		-- override common trigger.
		"c"
	}, {t"a or b"})
}, {
	key = "ab"
})

Also, opts is now passed through. I've thought a bit more about allowing multiple conditions via opts, but it feels wrong, since it's less clear which opts belong to which contexts

ms({"a", "b", {trig="c", hidden = true}}, {<some nodes>}, {{condition = cond_for_a}, {condition = cond_for_b}, {no_condition_for_c = nil}})

OTOH, if we now settle for only a single opts-table (so not a list of opts-tables), it'll be hard to add later 🥲

@L3MON4D3
Copy link
Owner Author

OTOH, if we now settle for only a single opts-table (so not a list of opts-tables), it'll be hard to add later

opts now provides optional argument to the function itself, one of which is the opts-table for all snippets (common_opts).
We can extend this with specific opts later, but I don't think it's important to have right now.

@L3MON4D3 L3MON4D3 force-pushed the multi_context branch 3 times, most recently from a16375e to 06ebc31 Compare March 6, 2023 16:59
@L3MON4D3 L3MON4D3 marked this pull request as ready for review March 6, 2023 17:03
@L3MON4D3 L3MON4D3 force-pushed the multi_context branch 2 times, most recently from 9cab739 to 0b8c50b Compare March 8, 2023 13:53
…add_snippets`.

only for internal usage for now, not api.
Since `init_context` is now responsible for creating a table which can
be added by add_snippets, has `invalidate`, `matches`, ...,
`invalidated` should be initilized in there.
don't reuse passed-in context-table, wrap context in string at
api-boundary, and not in internal methods (`init_snippet_context`).
@L3MON4D3 L3MON4D3 merged commit b405e6b into master Mar 8, 2023
Comment on lines +1056 to +1063
common = {snippetType = "autosnippet"},
{trig = "a", snippetType = "snippet"},
"b",
{trig = "c", condition = function(line_to_cursor)
return line_to_cursor == ""
end}
}, {
t"a or b (but autotriggered!!)"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't common not supposed to override settings specific to each entry? In that case, a will not be autoTriggered. or I am missing something?
Also, will be cool to account for C in the snippet?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, that snippet-string bit imprecise. In this case b and c would be autotriggered, but a would not. I'll amend that later👍

@danielo515
Copy link

@L3MON4D3 api looks great. I just added a little comment on PR for docs. Sorry for being that late

@L3MON4D3
Copy link
Owner Author

@L3MON4D3 api looks great. I just added a little comment on PR for docs. Sorry for being that late

Don't worry, thank you for taking a look :D

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.

Feature Request: Multiple Triggers for the same snippet
2 participants