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

feat(LSP-Snippets): add basic support for scope field #846

Closed
wants to merge 3 commits into from

Conversation

hinell
Copy link

@hinell hinell commented Mar 27, 2023

{
    "prefix"     : "...",
    "body"       : "var ${1:name} = ${2:value}",
    "scope"      : "javascript, typescript"
    "description": "My snippet"
}

The above give you an idea how you can specify language scope in which snippet should appear. This is primarily intended to be used inside *.code-snippets files. If you import scoped snippets by your package.json you may get duplicates, this PR prevents this.

See:
#705 (comment)

@leiserfg
Copy link
Contributor

I think it's better to implement the complete .code-snippet format instead.

@hinell
Copy link
Author

hinell commented Mar 28, 2023

I think it's just enough to have this PR to avoid snippet duplicates. The ordinary .json differs from .code-snippet by the latter having collections of differently scoped snippets. VS Code just loads all *.code-snippet files and assigns snippets per scoped language or GLOBALLY. It ignores <name> in <name>.code-snippet.

Additionally the LuaSnip code isn't intuitive, it doesn't follow OOP concept so for me it was hard to approach it quickly. In future this will prevent quick and easy modifications unless heavy refactoring is done. If you wanted to fetch all snippets from files and sort them out by scope for now it isn't easy task.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Mar 28, 2023

Tbh I think this code is fine for what it has to do, which is

  • find files belonging to some snippet-repository
  • either load them immediately, or add them to the files for some specific language, and load them when it is needed (lazy_load)

Of course this does not gel at all with changing the filetype of a snippet inside the loaded json, because we'd have to go all the way into the json-files and find the snippets which are meant for other filetypes :/
Thus, I agree with @leiserfg, we should add this in a separate module (or via different functions)

What's the general idea behind .code-snippets? Is it more project-local, or do you just dump all of your snippets in there?
Maybe a different approach: add support for .code_snippets in snippet-converter, that way they could also be converted to the (imo much easier to handle) snipmate-format, or to just a regular vscode snippet package, and then loaded by luasnip

@hinell
Copy link
Author

hinell commented Mar 28, 2023

@L3MON4D3 I've rebased this against v1.2.1, cause master is buggy. On master in LuaSnip it fails to load lua.code-snippets somehow. Didn't investigate, just saying.

Btw, is master used for future releases?

@leiserfg
Copy link
Contributor

What's the general idea behind .code-snippets?

It's for having personal snippets (not to be shared as a collection) that can be edited quickly in a single place without having to change a package.json every time one adds support for a new language.
Personally, I prefer to add those in a lua file but I can imagine users moving from vscode will like to preserve their old crafted snippets easily.

@L3MON4D3
Copy link
Owner

@L3MON4D3 I've rebased this against v1.2.1, cause master is buggy. On master in LuaSnip it fails to load lua.code-snippets somehow. Didn't investigate, just saying.

Btw, is master used for future releases?

Pretty sure I added some stuff to use a different parser for json/jsonc, there's not yet an option for .code-snippets, adding that might make it work.
And yes, master is used for future releases 👍

@L3MON4D3
Copy link
Owner

What's the general idea behind .code-snippets?

It's for having personal snippets (not to be shared as a collection) that can be edited quickly in a single place without having to change a package.json every time one adds support for a new language. Personally, I prefer to add those in a lua file but I can imagine users moving from vscode will like to preserve their old crafted snippets easily.

Ah, alright 👍
Not sure if the from_vscode-loader should also take care of these files (I think conceptually it should, but lazy_load does not make much sense for .code-snippets, since we have to read+parse the file anyway)

@leiserfg
Copy link
Contributor

Yes, it should be more like spliting load_snippet_files from the vscode-loader to be able to use the shared logic, and then make a loader -- maybe in the same module -- that only loads .code-snippets from a list of paths passed by hand by the user. IMHO it should not traverse the rtp as the other loaders and therefore should not have a lazy version.

@hinell
Copy link
Author

hinell commented Mar 28, 2023

because we'd have to go all the way into the json-files and find the snippets which are meant for other filetypes :/

@L3MON4D3 You don't, cause the list of paths to files containing snippets is mapped against language in package.json. This means that you don't traverse all the files, but only one path per several languages.

What's the general idea behind .code-snippets?

@L3MON4D3 The <name>.code-snippets just a collection of snippets that VS Code loads and sorts out by assigning them either based on scope of a snippet or to all if no scope is given. .code-snippets can be put in specific folders, like project local .vscode, checkout the link: https://code.visualstudio.com/docs/editor/userdefinedsnippets#_project-snippet-scope

Here is an example in my usecase:

package.json
{
	"name": "snippets",
	"contributes": {
		"snippets": [
              { "language": [ "javascript" ], "path": "./ecmascript.code-snippets" }
              { "language": [ "typescript" ], "path": "./ecmascript.code-snippets" }
              { "language": [ "typescriptreact" ], "path": "./ecmascript.code-snippets" }
		]
	}
}

@L3MON4D3
Copy link
Owner

@L3MON4D3 You don't, cause the list of paths to files containing snippets is mapped against language in package.json. This means that you don't traverse all the files, but only one path per several languages.

Isn't scope meant to set the language for a single snippet?
Or is it such that the snippet is only available in buffers belonging to some project (ie. they are in some subdirectory?) I can't really get much info from that link, sorry :/

@hinell
Copy link
Author

hinell commented Mar 28, 2023

Isn't scope meant to set the language for a single snippet?

Yes.

I use .code-snippets primarily to gather related snippets together if they are used across several languages, but not in the others. Imagine if I needed some css code in the javascript one. I would need to put snippets in different files: css.json and javascript.json. Updating them in different files would be a huge pain in the ass. Instead, I just use framework-x.code-snippets where all snippets are scoped. That's it.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Mar 28, 2023

Ah, okay 👍

Yeah in that case it really isn't straightforward to include in our current model, since we assume that one json file provides snippets for a single language, and can thus defer parsing the file until that language is needed.
If the json-snippets can just have that scope-field, which changes the filetype they are meant for, we can't do this lazy-loading, and have to parse all .code-snippets-files in their entirety (and should probably just add the snippets defined by them right away, doing otherwise seems like a waste), so that we know the filetype they are meant for. Or is their inherent mechanism that they are loaded only when some other filetype is loaded? If so, that sounds a bit unreliable

I also don't understand why the.code-snippets-files can be listed in a package.json, under some language. Isn't their whole idea to be able to quickly edit snippets (which having to modify a package.json kind of prevents?) and provide the filetype of some snippet in its definition?

@hinell
Copy link
Author

hinell commented Mar 28, 2023

to include in our current model, since we assume that one json file provides snippets for a single language

Well it's not your model, it's the VS Code one. This PR primarily aims at the backwardscompatibility, interoperability, and convenience. I have a bunch of snippets files I would like to reuse in neovim.

I also don't understand why the.code-snippets-files can be listed in a package.json,

In fact, VS Code reads all these files straight from the user ~/.config/code/user/snippets or project/.vscode subfolder. I'm specifying it because there is no way to tell LuaSnip to fetch these snipets. And I think this is the right way. Better than the VS Code one.

@L3MON4D3
Copy link
Owner

Well it's not your model, it's the VS Code one.

I'm just stating what the current implementation expects/how it behaves :)
We can of course extend this, and provide support for the .code-snippets-files, but it seems most practical to do so by implementing it in parallel, basically, and not inside the functions we currently use.

In fact, VS Code reads all these files straight from the user ~/.config/code/user/snippets or project/.vscode subfolder. I'm specifying it because there is no way to tell LuaSnip to fetch these snipets. And I think this is the right way. Better than the VS Code one.

I'm not certain, it seems more logical to just say "okay, look into this one file, there are a bunch of snippets and each tells you which language it belongs to", having the redirection over package.json seems unnecessary (while it does have a purpose in the snippet-collections where scope for changing the language is not allowed)

@hinell
Copy link
Author

hinell commented Mar 29, 2023

Merge this asap!

@L3MON4D3
Copy link
Owner

Why?

@hinell
Copy link
Author

hinell commented Mar 29, 2023

@L3MON4D3 What do you mean? This PR prevents LuaSnip from importing snippets not intended for current language. I think the entire thread above was just about that.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Mar 29, 2023

Oh, I misunderstood your intention with this PR a bit then, sorry about that.
If I understand the vscode-documentation correctly, scope is not allowed in non-global snippet files (ie. repositories, with a package.json), so it doesn't really make sense to include a check for it in the code loading those.

Doing this the right way would include implementing proper support for .code-snippets, since they have to be handled completely differently from the snippet-repositories loaded via load or lazy_load (essentially @leiserfg's first comment xD)

Are you interested in doing that?

@hinell
Copy link
Author

hinell commented Mar 29, 2023

is not allowed in non-global snippet files so it doesn't really make sense to include a check for it in the code loading those.

In VS Code it is allowed. scope is ignored when loading xyz.json file.

"Handled differently"

since they have to be handled completely differently from the snippet-repositories loaded via load or lazy_load (essentially @leiserfg's first comment xD)

NOT that differently, checkout this my reply again. This piece of PR code won't change if you implement *.code-snippets resolution mechanism for sure.

Proper support

Are you interested in doing that?

To make LuaSnip behavior compatible with VS Code, I think I can make a small fix to this PR that ensure that when .code-snippets files are evaulated by load() or lazy_load():

  • snippets without scope are loaded globally
  • snippets with scope loaded per buffer language

I dont' see any need for a separate codebase for .code-snippets resolution mechanism for now. I don't have time for that either. Its is much cheaper to specify it via package.json. More importantly: it won't break anything.

@L3MON4D3
Copy link
Owner

In VS Code it is allowed. scope is ignored when loading xyz.json file.

Okay, yes I meant ignored, of course it is allowed. vscode just doesn't do anything special if a snippet (loaded via package.json) sets scope, correct?

NOT that differently, checkout this my reply again. This piece of PR code won't change if you implement *.code-snippets resolution mechanism for sure.

Yes of course not that differently, we will be able to share much of the code, but load and lazy_load explicitly look for package.json and can load the files defined in it on-demand. And, afaict, this is where .code-snippets differs, since it is just a single file defining a bunch of snippets.

To make LuaSnip behavior compatible with VS Code, I think I can make a small fix to this PR that ensure that when .code-snippets files are evaulated by load() or lazy_load():

  • snippets without scope are loaded globally
  • snippets with scope loaded per buffer language

I'd rather have a separate function which takes the path to a .code-snippets (or looks for .vscode/ in some directories like cwd) than adding support for .code-snippets-with-package.json, which afaict isn't even supported in vscode, right?

I dont' see any need for a separate codebase for .code-snippets resolution mechanism for now.

I wasn't suggesting we add a separate codebase, just a different function exclusively responsible for loading .code-snippets-files

Its is much cheaper to specify it via package.json

Cheaper?

More importantly: it won't break anything.

It probably won't, but I'd rather have proper support on the first try, than a half-baked solution initially (which we'd have to support forever if we want to keep backwards-compatibility) and a good one, which makes the initial one obsolete, later.

@hinell
Copy link
Author

hinell commented Mar 29, 2023

just a different function exclusively responsible for loading

What's the point of this if you can do it via package.json, which is more logical? Btw, currently the package.json is used for VS Code extensions of snippets. I'm not sure whether I can specify code-snippets or not, but I think it's logical

I'd rather have a separate function which takes the path to a .code-snippets

You can outline the api?

which we'd have to support forever if we want to keep backwards-compatibility

There is a serious manpower shortage, you know! Let me think about that a bit! 😆

@L3MON4D3
Copy link
Owner

What's the point of this if you can do it via package.json, which is more logical?

It seems to be a completely different mechanism, the .code-snippets-files can just lay around anywhere afaict, if we want to support them properly, we can't require an associated package.json

Btw, currently the package.json is used for VS Code extensions of snippets

Yes! And I think that's different from the more user-centric .code-snippets

You can outline the api?

Sure, actually I think from_vscode.load_code_snippets(path-to-.code-snippets) might be enough.
With that, we would also have implicit support for the .vscode/*.code-snippets (actually, path would have to be a pattern for that to work) by calling require("luasnip.loaders.from_vscode").load_code_snippets(vim.fn.getcwd .. "/*.code-snippets")
(or should we allow passing a directory which is then searched for .code-snippets-files? That might be easier to understand than allowing path to be a pattern)

@hinell
Copy link
Author

hinell commented Mar 30, 2023

Yes! And I think that's different from the more user-centric .code-snippets

Just as a sidenote, per VS Code docs it's not prohibited to include .code-snippets into a package.json, see this and this link. Moreover, there is an official VS Code project template for the snippet extension that includes snippets.code-snippets into their package.json, which means that it's allowed. Take a look at this file for example: vscode-generator-code/generators/app/templates/ext-snippets

I've created a test-repo for the VS Code snippets extension and tested it: it works. It loads snippet.code-snippets via package.json. It ignores the scope field though which means this PR doesn't match VS Code behavior...

or should we allow passing a directory which is then searched for

Both dir and file paths are good to go I think.

@L3MON4D3
Copy link
Owner

Thank you for investigating 👍
I think all that's needed to support .code-snippets-files in package.json is to allow it here, make it use the jsonc-decoder (IIRC vscode allows them to be jsonc, that's why. Also the safer option :D)

Both dir and file paths are good to go I think.

👍
Concerning dir, I'm pretty sure it'd be enough to not recursively search the entire tree of directories starting from dir, but just the files immediately inside it (that would be enough to enable the .vscode-usage, I'm not sure if there are usecases where it makes sense to search all files inside a directory)

@hinell
Copy link
Author

hinell commented Mar 31, 2023

Is anyone interested in implementing new api?

@L3MON4D3
Copy link
Owner

Ahh a bit, I can take it on after finishing the PRs I should be finishing :D

@hinell
Copy link
Author

hinell commented Mar 31, 2023

@L3MON4D3 Which ones?

@L3MON4D3
Copy link
Owner

#826, #838, #846, I think then I'll tackle implementing .code-snippets-support :)
If you want to, you can make the change for loading .code-snippets from package.json in this PR, no need to wait with that

@hinell
Copy link
Author

hinell commented Apr 1, 2023

@L3MON4D3 I think I'm going to make some changes for sure. Let's see.

@hinell
Copy link
Author

hinell commented Apr 25, 2023

Been busy with buying new fridge. Any updated on this? Anyone started to work on new API?

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Apr 25, 2023

Cool (at least I hope it is) :P
I haven't progressed much (not in this PR), been busy with classes 😅

@hinell
Copy link
Author

hinell commented Apr 27, 2023

@L3MON4D3 Classes? What do you mean?

@L3MON4D3
Copy link
Owner

Oh, like going to class, at a school

@hinell
Copy link
Author

hinell commented Apr 28, 2023

Are you teaching someone how to use LuaSnip at chool? Just wondering

@leiserfg
Copy link
Contributor

No, he is learning how to quit vim.

@L3MON4D3
Copy link
Owner

Are you teaching someone how to use LuaSnip at chool? Just wondering

That'd be sick, but as far as I know I'm the only one using LuaSnip there 🥲

No, he is learning how to quit vim.

xD
Sooo close to wrapping my head around that :P

@L3MON4D3
Copy link
Owner

I got to it!
#894 should work, but haven't actually tested it yet 😅

@hinell hinell mentioned this pull request May 28, 2023
@L3MON4D3
Copy link
Owner

Implemented in #906

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