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

Support .code-snippets. #894

Closed
wants to merge 17 commits into from
Closed

Support .code-snippets. #894

wants to merge 17 commits into from

Conversation

L3MON4D3
Copy link
Owner

Both in package.json-style snippet-packages and as standalone files, where snippet-filetypes have to be set via scope.

@hinell
Copy link

hinell commented May 26, 2023

@L3MON4D3 I would say this is rather a rough solution... What's the purpose of the cache in short?

Lack of comments for that code actually prevents any sensible contributions.

@L3MON4D3
Copy link
Owner Author

L3MON4D3 commented May 26, 2023

Oh, why would you say this is rough? And where do you think more comments would help?
Good point about the cache, I forgot to put in the part where it actually re-uses the snippets it loaded previously (although, maybe that's not actually necessary for .code-snippets, I guess loading the same file twice shouldn't occur, and is most likely an error)

@hinell
Copy link

hinell commented May 26, 2023

Why not every s(....) takes a separate option filetype? I think it should be an option.

@hinell
Copy link

hinell commented May 26, 2023

Well, I still think we should be able to have .code-snippets listed in the package.json to be picked up by the .lazy_load(...) / load().

I wouldn't want to config snips paths in different places (some lua file, etc). It's logically reasonable to have paths in package.json.

@L3MON4D3
Copy link
Owner Author

Why not every s(....) takes a separate option filetype? I think it should be an option.

.code-snippets is the only source which provides snippets for multiple filetypes from one file :/
I don't think it's worth it to move that complexity into add_snippets

@L3MON4D3
Copy link
Owner Author

Well, I still think we should be able to have .code-snippets listed in the package.json to be picked up by the .lazy_load(...) / load().

I wouldn't want to config snips paths in different places (some lua file, etc). It's logically reasonable to have paths in package.json.

Oh, that's (or should be, at least) implemented via 6a46f76

@L3MON4D3
Copy link
Owner Author

L3MON4D3 commented May 27, 2023

Why not every s(....) takes a separate option filetype? I think it should be an option.

.code-snippets is the only source which provides snippets for multiple filetypes from one file :/ I don't think it's worth it to move that complexity into add_snippets

Although, on second thought, it wouldn't lead to much more complexity, and would be pretty cool for multisnippet..... Hmmmmmmm
The issue is that reloading lazy-loading would become waaaaay a bit more complicated, since now any file may contribute snippets for any filetype

@hinell
Copy link

hinell commented May 27, 2023

The issue is that reloading lazy-loading would become waaaaay a bit more complicated

It ain't. It's logical for a snippet to have an associated file type. This way the function that applies snippets may sort them out in advance and update them on demand.

@L3MON4D3
Copy link
Owner Author

It's also logical to group ones own snippets by filetype :P
I'm not sure what you mean by "sort them out and update them on demand".
The issue im talking about is: we want to lazy-load snippets, which means that snippets are only loaded when they are actually required, for example when a buffer with their filetype is edited. With package.json-snippets, this works pretty good because that file describes which filetype any file contributes snippets to, and we can thus delay reading that file until it's filetype is actually needed.
If now any snippet in that file declares that it belongs to a different filetype, that snippet would not be loaded in time, and would be missing.
I'm sure there are ways around that, but imo the vscode-loader covers most common scenarios, and for anything more complicated, lua should be able to handle it.

@hinell
Copy link

hinell commented May 27, 2023

Warning: SEE THIS REPLY, IT TURNS OUT YOU CAN'T USE ARRAYS IN language field.

I'm sure there are ways around that,

@L3MON4D3 This is why I'm advocating the package.json-centric approach. With package.json specified paths, it's easy to do async/lazy/postponed/ondemand snippets loading. This optimization though is not a performance issue IMO. Dunno why you are caring so much about it.

E.g.:

snippets/package.json

{
	"name": "snippets",
	"contributes": {
		"snippets": [
		...
		, { "language": [ "markdown" ], "path": "./markdown.code-snippets" }
		...
		]
	}
}

@L3MON4D3
Copy link
Owner Author

I don't understand what that example is supposed to show me, could you explain in more detail?

@L3MON4D3
Copy link
Owner Author

But, maybe do so in another issue if it's actually about another feature, this is getting off-topic I think

@hinell
Copy link

hinell commented May 28, 2023

I don't understand what that example is supposed to show me

I made a small mistake. It's supposed to show you how to load .code-snippets via package.json, not load_standalone function.

@L3MON4D3
Copy link
Owner Author

Ah, okay
Loading just files with the extension .code-snippets is possible, what (fundamentally) won't work is that the file advertises filetypes A, B, C in package.json, and has snippets that have scope set to filetype D (because then lazy_load won't load that file for filetype D, and the snippet will be missing).
If you're advocating for scope-support in package.json-loaded .code-snippets, what is the advantage you see?
One could just as well split the .code-snippets-file into multiple .json(c), without scope (if that seems like too much work, have you considered using the snipmate-format? It has much less boilerplate, and vscode-snippets can be converted to it with snippet-converter.nvim)

@hinell
Copy link

hinell commented May 28, 2023

won't load that file for filetype D, and the snippet will be missing

Not a problem. The package.json can easily give a hint to the snippet engine on what kind of file-type scoped snippets it should import from, e.g:

{ "language": [ "A", "B", "C", "D" ], "path": "./tool-specific.code-snippets" }

, what is the advantage you see?

I told you elsewhere alredy that I have a collection of snippets that I've separated by specific tools and purpose. It would be outright stupid to keep all these in one single per-language *.json. Because of that I even plan to convert all my json files to yml for easier editing.

snipmate

I prefer to keep my snips in a cross-platform format actually. Except for LuaSnip, which is kinda different. Would be best if it would b 90% convertible to vscode/textmate format in the future.

@L3MON4D3
Copy link
Owner Author

Ah okay, okay, now I'm convinced😅
Hadn't kept the reason for all that in mind since the last time we discussed this.
I guess the way to explain this feature would just be that scope further filters the filetypes given in package.json. How does vscode behave if the scope-filetype is not one of those?

@hinell
Copy link

hinell commented May 28, 2023

How does vscode behave if the scope-filetype is not one of those?

Replied elsewhere already. (scope is ignored afaik)

@L3MON4D3
Copy link
Owner Author

L3MON4D3 commented May 28, 2023

Oh, I kinda though I must've misunderstood something there, because you were talking about wanting to have compatibility/a standard format.
IIUC a collection which relies on this scope-mechanism to filter snippets can't be used with vscode then, right?

@L3MON4D3
Copy link
Owner Author

Oh, language as an array is used pretty extensively by friendly-snippets, that alone makes it worth supporting (have you tested if vscode accepts it?)

@hinell
Copy link

hinell commented May 28, 2023

BTW, if you want 100% compatibility the VS Code it says that the package.json#contributions.language field should be a of string type, NOT an array. It turns out there is embeded schema for that. VS Code says it's error to use array (I made an extension to test it):
image

You should use empty string instead as there is no all language identifier for that field to get snippets filtered based on scopes:

{
  "contributes": {
    "snippets": [
      {
        "language": "",
        "path": "./snippets/foo.code-snippets"
      }
    ]
  }
}

This way snippets are imported and filtered based on scope field.

  • (have you tested if vscode accepts it?)*

It's likely won't accept it. See the first screenshot (spot the error).

@hinell
Copy link

hinell commented May 28, 2023

I think making LuaSnip 100% compatible with VS Code extension isn't important. Though, the recent findings make it hard to hint engine on what kind of snippets .code-snippets contains. Anyway, I don't think it's worth even discuss this optimization (i.e. language-aware lazy loading) as all these files are very small and there is no evidence there is a performance impact on loading. Unless of course you can prove me wrong.

@L3MON4D3
Copy link
Owner Author

Wait, so if language = "", vscode does look at scope? Ewww :(

@L3MON4D3
Copy link
Owner Author

Anyway, I don't think it's worth even discuss this optimization (i.e. language-aware lazy loading) as all these files are very small and there is no evidence there is a performance impact on that. Unless of course you can prove me wrong

Oh you can easily test that, load something like friendly-snippets via load and then compare that with lazy_load. There's enough of a difference that I wouldn't want the former to run during startup

@hinell
Copy link

hinell commented May 28, 2023

We can use the following hinting approach (using the same file twice):

{
  "contributes": {
    "snippets": [
      {
        "language": "go",
        "path": "./snippets/collection.code-snippets"
      },
      {
        "language": "rust",
        "path": "./snippets/collection.code-snippets"
      },

    ]
  }
}

This way engine may figure to use cached file to avoid loading it twice and filter snippets based on scope.

Oh you can easily test that...

load()ing friendly-snippets is slow indeed. Is there a compilation step for snippets that LuaSnip performs? Just wondering. 🤔

@L3MON4D3
Copy link
Owner Author

load()ing friendly-snippets is slow indeed. Is there a compilation step for snippets that LuaSnip performs? Just wondering. 🤔

Hah, it used to be way worse. We used to parse the snippets outright, this is just reading+parsing json and adding them a kind of proxy, which parses them on expand

This way engine may figure to use cached file to avoid loading it twice and filter snippets based on scope.

Filtering with scope should be fine, doesn't seem to introduce much more complexity.
I'll look into that after getting this here done.

@L3MON4D3
Copy link
Owner Author

Decoupling the filetype(s) which trigger a lazy_load from the actual filetype of the snippet is a really nice idea.

One cool thing that is possible with that is setting the language in package.json to some framework, and then scope each snippets to css, html, js, whatever.
One could then modify load_ft_func to load the filetype framework as soon as a file using that framework is loaded, which will cause our loaders to actually load all snippets associated with that framework.
This load_ft_func-modification was already possible before, but requires that snippets are split up into filetypes like framework1-css, framework1-html, ..., which may bring some cognitive overhead as opposed to having everything in one file (but might be a bit faster, since everything can be loaded even more lazily)
I was a reluctant to implement this because I thought it would require lots of recursive
copying of snippets, but that is actually not the case

I'm going to implement this here, since it simplifies the implementation of load_standalone, seems weird to merge this and then change it immediately.

@hinell
Copy link

hinell commented May 28, 2023

@L3MON4D3 What's the bottleneck of loading snippets?

@L3MON4D3
Copy link
Owner Author

I suspect that right now, it might be copying around of snippets, but I'm working on that right now
If you want to figure it out, try generating a flamegraph of load or lazy_load

@L3MON4D3
Copy link
Owner Author

Continued in #906

@L3MON4D3 L3MON4D3 closed this May 29, 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.

Load workspace setting snippets from .vscode dir
2 participants