-
-
Notifications
You must be signed in to change notification settings - Fork 233
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
Conversation
cd8b2c7
to
2909a27
Compare
I think it's better to implement the complete |
I think it's just enough to have this PR to avoid snippet duplicates. The ordinary 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 |
bcadaf4
to
10f54b9
Compare
Tbh I think this code is fine for what it has to do, which is
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 :/ What's the general idea behind |
094ed87
to
95e6a1a
Compare
@L3MON4D3 I've rebased this against v1.2.1, cause master is buggy. On master in LuaSnip it fails to load Btw, is master used for future releases? |
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 |
Pretty sure I added some stuff to use a different parser for json/jsonc, there's not yet an option for |
Ah, alright 👍 |
Yes, it should be more like spliting |
@L3MON4D3 You don't, cause the list of paths to files containing snippets is mapped against
@L3MON4D3 The 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" }
]
}
}
|
Isn't |
Yes. I use |
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. I also don't understand why the |
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.
In fact, VS Code reads all these files straight from the user |
I'm just stating what the current implementation expects/how it behaves :)
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 |
Merge this asap! |
Why? |
@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. |
Oh, I misunderstood your intention with this PR a bit then, sorry about that. Doing this the right way would include implementing proper support for Are you interested in doing that? |
In VS Code it is allowed. "Handled differently"
NOT that differently, checkout this my reply again. This piece of PR code won't change if you implement Proper support
To make LuaSnip behavior compatible with VS Code, I think I can make a small fix to this PR that ensure that when
I dont' see any need for a separate codebase for |
Okay, yes I meant ignored, of course it is allowed. vscode just doesn't do anything special if a snippet (loaded via
Yes of course not that differently, we will be able to share much of the code, but
I'd rather have a separate function which takes the path to a
I wasn't suggesting we add a separate codebase, just a different function exclusively responsible for loading
Cheaper?
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. |
What's the point of this if you can do it via
You can outline the api?
There is a serious manpower shortage, you know! Let me think about that a bit! 😆 |
It seems to be a completely different mechanism, the
Yes! And I think that's different from the more user-centric
Sure, actually I think |
Just as a sidenote, per VS Code docs it's not prohibited to include I've created a test-repo for the VS Code snippets extension and tested it: it works. It loads
Both dir and file paths are good to go I think. |
Thank you for investigating 👍
👍 |
Is anyone interested in implementing new api? |
Ahh a bit, I can take it on after finishing the PRs I should be finishing :D |
@L3MON4D3 Which ones? |
@L3MON4D3 I think I'm going to make some changes for sure. Let's see. |
Been busy with buying new fridge. Any updated on this? Anyone started to work on new API? |
Cool (at least I hope it is) :P |
@L3MON4D3 Classes? What do you mean? |
Oh, like going to class, at a school |
Are you teaching someone how to use LuaSnip at chool? Just wondering |
No, he is learning how to quit vim. |
That'd be sick, but as far as I know I'm the only one using LuaSnip there 🥲
xD |
I got to it! |
Implemented in #906 |
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 yourpackage.json
you may get duplicates, this PR prevents this.See:
#705 (comment)