search

davidgranstrom / scnvim

Neovim frontend for SuperCollider.
GNU General Public License v3.0
197 stars 26 forks source link

Luasnip support #152

Closed madskjeldgaard closed 2 years ago

madskjeldgaard commented 2 years ago

This adds LuaSnip support for the auto generated snippets in SCNvim and closes https://github.com/davidgranstrom/scnvim/issues/151

The snippets generated are compatible with nvim-cmp (because they use the name and dscr tags).

salkin-mada commented 2 years ago

Nice!

madskjeldgaard commented 2 years ago

screenshot-25-10-21-20_37 9

davidgranstrom commented 2 years ago

@madskjeldgaard Thanks, I'll try it out!

davidgranstrom commented 2 years ago

Hey @madskjeldgaard,

I've tried this locally now and it works well!

Should the snippets have spaces in between the arguments do you think? This is how the other snippets have been generated at least, but I get that it might be a personal preference. I think at some point we should allow users to specify this somehow anyways, perhaps along with other options like if we want to keep the keyword arguments or use them just as placeholders for the snippet, so its fine by me with the current implementation. Let me know what you think and we can proceed to get this merged! I think all extra configuration should be addressed in a different PR, I have some thoughts about it so I could make an issue.

SinOsc.ar(freq:440.0,phase:0.0,mul:1.0,add:0.0) // current

SinOsc.ar(freq:440.0, phase:0.0, mul:1.0, add:0.0) // reference
madskjeldgaard commented 2 years ago

Hey @madskjeldgaard,

I've tried this locally now and it works well!

Should the snippets have spaces in between the arguments do you think? This is how the other snippets have been generated at least, but I get that it might be a personal preference. I think at some point we should allow users to specify this somehow anyways, perhaps along with other options like if we want to keep the keyword arguments or use them just as placeholders for the snippet, so its fine by me with the current implementation. Let me know what you think and we can proceed to get this merged! I think all extra configuration should be addressed in a different PR, I have some thoughts about it so I could make an issue.

SinOsc.ar(freq:440.0,phase:0.0,mul:1.0,add:0.0) // current

SinOsc.ar(freq:440.0, phase:0.0, mul:1.0, add:0.0) // reference

Yes that was a personal preference that snuck it's way in here.

I agree that this should be an option alongside the ability to break each argument into it's own newline. We can make a seperate PR for these ideas. I think that generally the generateSnippets method is getting a bit dense and should be broken down in smaller methods to make it more readable before we do this. Just my opinion.

I'll add spaces and update the PR :)

molleweide commented 2 years ago

This is pretty badass!

madskjeldgaard commented 2 years ago

Should be updated now @davidgranstrom Thanks @molleweide :)

davidgranstrom commented 2 years ago

I agree that this should be an option alongside the ability to break each argument into it's own newline. We can make a seperate PR for these ideas. I think that generally the generateSnippets method is getting a bit dense and should be broken down in smaller methods to make it more readable before we do this. Just my opinion.

@madskjeldgaard you read my thoughts :)

I'll add spaces and update the PR :)

Thanks! It looks great. Since this PR provides immediate user value I'll merge it now, and then we'll make a plan for refactoring going forward.