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 meta data for each add_snippets call #719

Closed
wants to merge 16 commits into from

Conversation

molleweide
Copy link

@molleweide molleweide commented Jan 8, 2023

  • implements basis for adding meta data to snippets w/new config param.
  • improves available() to be a bit more versatile.
  • adds jump_to_snip() under extras

@molleweide molleweide changed the title meta data add meta data for each add_snippets call Jan 8, 2023
@molleweide
Copy link
Author

It seems that the CI removed my additions to the doc/luasnip.txt file. 47a669d

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Jan 8, 2023

Ah, yeah add doc to DOC.md, doc/luasnip.txt is auto-generated from it 😅

@molleweide
Copy link
Author

Ah, yeah add doc to DOC.md, doc/luasnip.txt is auto-generated from it 😅

fixed it

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Jan 8, 2023

Nice 👍

@@ -24,6 +24,7 @@ local defaults = {
region_check_events = "User None",
delete_check_events = "User None",
store_selection_keys = nil, -- Supossed to be the same as the expand shortcut
store_meta_data = false, -- For each snippet, eg. source path.
Copy link
Owner

Choose a reason for hiding this comment

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

I think I'd like this to be a table, which can accept keys to determine what is stored. Most capable would be just a function which receives all available data and creates the metadata for the snippet, but I think this is overdoing it, since we probably don't need much flexibility there.

Still, making this a table would somewhat future-proof it, like

metadata = {sourcefile=true, line=true, custom = somefn}

Copy link
Author

@molleweide molleweide Jan 8, 2023

Choose a reason for hiding this comment

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

I think using a subtable is good idea. I don't understand your reasoning about the function but it makes sense to do this properly in case we come up with more use cases.

Copy link
Owner

Choose a reason for hiding this comment

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

Nice, thanks!
My idea with the function was to allow complete customization of what is put into the metadata, essentially by calling a function like

function loader_metadata(filename, line)
	return {source=filename, source_line = line}
end

in the loaders.

Since there isn't this much customization possible (I think), letting the user set some keys for what exactly should be stored in the metadata-table seems like a more straightforward, and easier to use simplification.

Copy link
Author

Choose a reason for hiding this comment

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

Ill try to making it into a func and see how it feels

Copy link
Author

Choose a reason for hiding this comment

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

Now, this is the only thing left to do.

Copy link
Owner

Choose a reason for hiding this comment

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

How would one get the source line of the snippet in this function? Is there something builtin to luasnip already?

Oh, that's very hard to do for lua and vscode, I used it because I wanted an example for why more flexibility might be good.
I think we should not determine all lines up-front (except if it's trivial, like in the snipmate-loader), since only a tiny subset of this information will be useful, and it should, I think, be calculated on-demand

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe not very hard, non-trivial

Copy link
Owner

Choose a reason for hiding this comment

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

Also, the location for where the meta data loader should be called is within the session.snippets_colletion right, so we need to local session = require("luasnip.session") and then pass along snippets and opts to store_meta_data().

I had in mind that add_snippets receives the complete metadata (which may be nil), and we make the config-option specific to the loaders

Copy link
Author

Choose a reason for hiding this comment

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

Maybe not very hard, non-trivial

Aight, for now my feed keys thingy works good nuff

Copy link
Owner

Choose a reason for hiding this comment

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

Right! heuristics like that should do it for 99%

* upstream/master:
  Auto generate docs
  Auto generate docs
  docs(extras): add Snippet List
  feat(extras): added open_snippet_list
opts.scope allows one to either retrieve:

1. buf snips
2. snips by list of requested filetypes
3. all possible snips if scope == "everything"

opts.include_autosnippets
@molleweide
Copy link
Author

molleweide commented Jan 9, 2023

  1. I undid my changes to the default_snip_info func now that I pulled your latest changes.
  2. I added an opts param to the ls.available() func so that one can retrieve snippets with a little bit more flexibility.

Now you can either:

  1. retrieve snips for buf
  2. .... all snips
  3. .... only a subset of filtypes if you pass opts.scope = { "ft1", "ft2" }

@molleweide
Copy link
Author

Oh i realise that I merged master instead of rebasing. Ill have to look into how I can undo this later.

@molleweide
Copy link
Author

i created a base for using a user function when loading meta data. check it out and tell me what you think.

@molleweide
Copy link
Author

molleweide commented Jan 9, 2023

Also feel free (not important...) to try my picker that I linked to under the issue discussion. it shows you how I use all of the features in the available() func.

@L3MON4D3
Copy link
Owner

  1. I undid my changes to the default_snip_info func now that I pulled your latest changes.
  2. I added an opts param to the ls.available() func so that one can retrieve snippets with a little bit more flexibility.

Now you can either:

  1. retrieve snips for buf
  2. .... all snips
  3. .... only a subset of filtypes if you pass opts.scope = { "ft1", "ft2" }

I think we can use get_snippets for this usecase? (Don't really want to tack more onto available, we kind of messed up its design by having it take an optional argument directly, and not in an opts-table🤦)

@L3MON4D3
Copy link
Owner

I'll take a look at the other changes later😅

@molleweide
Copy link
Author

I'll take a look at the other changes later😅

Aight no stress!

@molleweide
Copy link
Author

I think we can use get_snippets for this usecase? (Don't really want to tack more onto available, we kind of messed up its design by having it take an optional argument directly, and not in an opts-table🤦)

Cool yeah im down for whatever. I'll wait untill you've had a closer look and then I modify it accordingly.

@@ -113,6 +113,30 @@ local function match(index, _match, _then, _else)
return F(func, index)
end

local function jump_to_snip(snip)
Copy link
Owner

Choose a reason for hiding this comment

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

I think this deserves its own module, then we can cleanly provide different heuristics for finding the correct snippet (after making that configurable ofc :D)
For example, finding in snipmate/lua/vscode could be made more precise by writing it specifically for that type of snippet

Copy link
Author

Choose a reason for hiding this comment

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

ok. i'll move this into its own module.

@@ -63,24 +63,66 @@ local function default_snip_info(snip)
}
end

local function available(snip_info)
Copy link
Owner

@L3MON4D3 L3MON4D3 Jan 10, 2023

Choose a reason for hiding this comment

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

Okay afaict this can also be achieved with either the old available (scope=buf) or get_snippets (everything else). Even though this design is nice, separate functions are backward compatible, so I think we should stick with that

Copy link
Author

Choose a reason for hiding this comment

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

quick note: my addition should be backwards compatible.
I'll get back with another reply later.

Copy link
Author

@molleweide molleweide Jan 10, 2023

Choose a reason for hiding this comment

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

Allright, so I think I understand what you mean but could you tell me a bit more specifically what you'd like me to do. Is my modification worth keeping in a modified state or should we scrap it completely?

My idea was that it made sense to put the logic in available so that one could filter all possible combinations via snip_info but I am also down with splitting it up somehow.

Copy link
Owner

Choose a reason for hiding this comment

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

True, of course it is backward compatible 🤦
What I don't like about this is that there's no good reason for snip_info to not be a part of opts, except staying backward compatible.

I think we'll have to scrap it :/
An alternative would be to expose it in addition to available (and implement available using it), but all of its functionality can be covered better by get_snippets (since get_snippets can access the implementation of snippet_collection)

Copy link
Author

@molleweide molleweide Jan 10, 2023

Choose a reason for hiding this comment

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

An alternative would be to expose it in addition to available (and implement available using it), but all of its functionality can be covered better by get_snippets (since get_snippets can access the implementation of snippet_collection)

Yeah this makes sense. I'll try to add a new successor func that only uses the opts param. I'm thinking about calling it filter_snippets(opts) or retrieve_snippets(opts).

local snippets_meta_data = {}
local snip_id_to_meta_map = {}

local function initialize_snippet_meta_data(opts)
Copy link
Owner

Choose a reason for hiding this comment

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

Okay, I think this mechanism is good, but we need to split it up a bit:

  • config generates a function which, given a filename produces the metadata that shall be added to snippets loaded by one of the loaders. We should not pass the filename directly, but wrap it in a table like {filename=<the filename>}, since we can then add more arguments and the function will stay comfortable to use (no need to write a handler like function(_, _, _, theOneArgINeed) ... end)
  • this function is used in loaders to generate the metadata for the snippets (no ifs or something like that, we can just call the function. This is flexible enough to probably not require any changes, except if new parameters are added)
  • add_snippets receives metadata-table, which is associated with the snippets added in this call, unconditionally

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I have to ponder your reply and come back later with a revised version.

Copy link
Owner

Choose a reason for hiding this comment

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

Looking forward to it :D
If you have any questions or other ideas, go ahead, I'd love to find something better (especially the association of snippet with its metadata is a bit annoying, since we have to do that id-redirection. Can't think of a better way though :/)

Copy link
Author

@molleweide molleweide Jan 10, 2023

Choose a reason for hiding this comment

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

Cool, yeah I cant really come up with something better either atm.
Let's optimize it later and I'll stick to the id mapping for now.

@L3MON4D3
Copy link
Owner

Hey!
I just found out about debug.getinfo, we could use this to accurately get the lines a snippet is defined on (for lua!!!).
With this, and snipmate working nicely (ie. we can get the lines the snippet is defined on), I have to admit that snippet-specific metadata is probably the way to go, and wouldn't mind just setting snippet.metadata (though I'd still like an API through snippet_collection, that would allow us to replace this later on)

Are you still interested in working on this? No worries if not, I'd be up for getting this done

@L3MON4D3
Copy link
Owner

Was feeling a bit too motivated the last few days, couldn't resist doing some work on this 😅
Check #826, I'd love to hear your thoughts :D

@molleweide
Copy link
Author

molleweide commented Mar 18, 2023

Hey mate!!
Wow, I gotta checkout what you've done. I haven't spent a lot of time on this since we last spoke - i just haven't had time + plus too many macos issues with vim so there has been some other fires ive had to try to put out 🙄 (i have to migrate over to linux asap)
I'll look at what you did and see if I can come with some valid feedback. I'm really excited to hear that you got something good working. :)
If I can get back on track I would sure love to help out a bit more but atm I have a bit of a hurdle to get past.

edit

Dude, you added the feature where one can jump to the source from within an opened snippet - that is pretty damn cool.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Mar 18, 2023

I haven't spent a lot of time on this since we last spoke - i just haven't had time + plus too many macos issues with vim so there has been some other fires ive had to try to put out 🙄 (i have to migrate over to linux asap)

Phuh, that's rough😅

I'll look at what you did and see if I can come with some valid feedback. I'm really excited to hear that you got something good working. :)

👍👍

If I can get back on track I would sure love to help out a bit more but atm I have a bit of a hurdle to get past.

Take your time, I don't have much to do right now, so plenty of opportunity to work on this :)

Dude, you added the feature where one can jump to the source from within an opened snippet - that is pretty damn cool.

Yeah! Really happy that worked out so well :D

@molleweide
Copy link
Author

Yeah! Really happy that worked out so well :D

Has it already improved your workflow?

@L3MON4D3
Copy link
Owner

Nah xD
But it was fun to write!
And it feels like such a nice addition, I'm sure it'll come in handy sometimes

@leiserfg
Copy link
Contributor

But it was fun to write!

Reminds me that after all the work in LuaSnip, I have just like 5 custom snippets 😄

@L3MON4D3
Copy link
Owner

Reminds me that after all the work in LuaSnip, I have just like 5 custom snippets 😄

😆😆
You're just using friendly-snippets, right? I guess that +lsp covers almost anything one would practically need, most snippets beyond that are just eye(/finger?/brain? :D)-candy

@molleweide
Copy link
Author

But it was fun to write!

lol

@L3MON4D3
Copy link
Owner

L3MON4D3 commented May 1, 2023

I've finally merged #826, closing this as well.
Sorry we couldn't use your work :/

@L3MON4D3 L3MON4D3 closed this May 1, 2023
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