Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(extras): added open_snippet_list #652

Merged
merged 4 commits into from Jan 8, 2023
Merged

feat(extras): added open_snippet_list #652

merged 4 commits into from Jan 8, 2023

Conversation

ghost
Copy link

@ghost ghost commented Nov 8, 2022

Implements functionality discussed in #607

Here's a showcase of what it looks like:
snippet_list

A couple of thoughts:

The available and the get_context function are essentially the same as the ones in luasnip/init.lua the only difference is adding the docstring in get_context and using that new get_context in the available function. There might be a way to refactor something so that there is less code repetition if that's something you would be interested in.

I'm not great at regexes so I did the best I could with the folding stuff haha. I think it's fine right now but it can be a little finnicky, for example if you try to use telescopes buffer fuzzy finder in that window you can get something like this:
image

At first I was just looking to open a new buffer and making that the current one, but making so that folding is available makes that hard since it's a per window and not a per buffer setting. So I went back to the vertical split implementation.

Let me know what you think, if everything seems good to you the only thing missing would be adding it to the documentation.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Nov 8, 2022

Nice!

The available and the get_context function are essentially the same as the ones in luasnip/init.lua the only difference is adding the docstring in get_context and using that new get_context in the available function. There might be a way to refactor something so that there is less code repetition if that's something you would be interested in.

Oh, yes that would be great. Could extract the common part of getting available snippets, and then have the two functions collecting the information depend on that.

I'm not great at regexes so I did the best I could with the folding stuff haha. I think it's fine right now but it can be a little finnicky, for example if you try to use telescopes buffer fuzzy finder in that window you can get something like this:

regex 🤮 :P
Doesn't this example with the fuzzy finder always occur in nested folds? (independent of foldmethod? or are the folds not nested correctly?)
An alternative approach would be manual folding, then we'd need to take over part of vim.inspect's job to reliably find the lines corresponding to each filetype

At first I was just looking to open a new buffer and making that the current one, but making so that folding is available makes that hard since it's a per window and not a per buffer setting. So I went back to the vertical split implementation.

Good choice, very unfortunate that folding is per-window, that's a bit annoying (at least for us :D)

@ghost
Copy link
Author

ghost commented Nov 9, 2022

The available and the get_context function are essentially the same as the ones in luasnip/init.lua the only difference is adding the docstring in get_context and using that new get_context in the available function. There might be a way to refactor something so that there is less code repetition if that's something you would be interested in.

Oh, yes that would be great. Could extract the common part of getting available snippets, and then have the two functions collecting the information depend on that.

Yeah! I don't know if you want to do this in this PR or in a separate one.

I'm not great at regexes so I did the best I could with the folding stuff haha. I think it's fine right now but it can be a little finnicky, for example if you try to use telescopes buffer fuzzy finder in that window you can get something like this:

regex 🤮 :P Doesn't this example with the fuzzy finder always occur in nested folds? (independent of foldmethod? or are the folds not nested correctly?) An alternative approach would be manual folding, then we'd need to take over part of vim.inspect's job to reliably find the lines corresponding to each filetype

Yeah regex makes me puke too :(. Seems that setting the foldmethod to indent fixed everything. I never use folding so I'm in uncharted waters here haha. If foldmethod set to indent is not enough I'll take a look into manual folding (I hope indent is enough lol)

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Nov 9, 2022

Yeah! I don't know if you want to do this in this PR or in a separate one.

Doing that in this one should be fine, we introduce the potential for this abstraction here, I think that justifies it :D

Seems that setting the foldmethod to indent fixed everything. I never use folding so I'm in uncharted waters here haha. If foldmethod set to indent is not enough I'll take a look into manual folding (I hope indent is enough lol)

Yeah, manual folding would probably be overcomplicating it 👍

@ghost
Copy link
Author

ghost commented Nov 10, 2022

Doing that in this one should be fine, we introduce the potential for this abstraction here, I think that justifies it :D

Sounds good! I would like to get some of the stuff above resolved first since I feel stuff is piling up. But my initial thoughts on this was to 'inject' the get_context into the available function so something like:

This would be in luasnip/init.lua

function available(get_context)
    get_context = get_context or 'default function here'
end

Then we can use whatever get_context function we want while keeping the available function untouched.

@L3MON4D3
Copy link
Owner

Doing that in this one should be fine, we introduce the potential for this abstraction here, I think that justifies it :D

Sounds good! I would like to get some of the stuff above resolved first since I feel stuff is piling up. But my initial thoughts on this was to 'inject' the get_context into the available function so something like:

This would be in luasnip/init.lua

function available(get_context)
    get_context = get_context or 'default function here'
end

Then we can use whatever get_context function we want while keeping the available function untouched.

Oh yeah, nice!! 👍

@ghost
Copy link
Author

ghost commented Nov 10, 2022

(Sorry for the force pushes lol)

Ok so I bring some updates! Hopefully I'm heading in the right direction here

Changes

luasnip/init.lua

In the available function I added a opts parameter which resolves to opts = opts or {get_context = get_context} that way we use the get_context that is defined in that file by default. Now that I'm thinking about it I think a better way would be to do

opts = opts or {}
opts.get_context = opts.get_context or get_context

Using the former would mean that if someone passes in an empty object for example available({}) or any object available({blahblah = 'hihi'}) opts wouldn't have any get_context entry. I'll update that.

Edit: Done in 22e2370

But with that we have flexibility to use whatever get_context function we want

extras/snippet_list.lua

I added an opts parameter to the open_snippet_list which resolves to

opts = opts or {
    get_name = get_name,
    printer = printer,
    win_opts = win_opts,
    buf_opts = buf_opts
}

Same issue was above, it might be better to do something like

opts = opts or {}
opts.get_name = opts.get_name or get_name
etc...

Edit: Done in 22e2370

get_name function

It's as we discussed, asimple function that takes in a bufnr and returns the name for the buffer, you can pass in whatever function you would like that returns a string

printer function

Takes in snippets and returns a string. At the moment we are only using vim.inpect

win_opts

Table of extra options to be set on the window. set_win_opts sets these options.

I use these 2 to set the folding at the moment

buf_opts

Table of extra options to set on the buffer. set_buf_opts sets these options.

I use these 2 to set the filetype to lua. (I know you want something like nvim tree but in the meantime I kept this here just to get syntax highlighting)

make_scratch_buffer function

Just moved the setting options for the scratch buffer to this function to clean up the open_snippet_list function

So hopefully this is looking more like you were picturing in terms of configuration capability.

Discussion Points

Pretty Print Stuff

Similar to nvim-tree, but not of files and directories, but of snippets and file/snippet-types.

I think I get it now! Similar to what packer does when you sync/update plugins. Gotcha. That would definitely be awesome, atm it's a little over my head though. I'm not sure how nvim-tree or packer make stuff collapsible and get highlighting/icons for example. I would have to go dig in their repos and see what they are doing.

In the meantime I left the folding method and the filetype to lua (For syntax highlighting purposes). With the configuration options we have now though it would be really easy to add whatever pretty-printer/buffer options/window options that you would like.

@L3MON4D3
Copy link
Owner

(Sorry for the force pushes lol)

Oh, do not worry about that at all

luasnip/init.lua

In the available function I added a opts parameter which resolves to opts = opts or {get_context = get_context} that way we use the get_context that is defined in that file by default. Now that I'm thinking about it I think a better way would be to do

opts = opts or {}
opts.get_context = opts.get_context or get_context

Using the former would mean that if someone passes in an empty object for example available({}) or any object available({blahblah = 'hihi'}) opts wouldn't have any get_context entry. I'll update that.

Edit: Done in 22e2370

Nice! 👍
WDYT about renaming it? I think get_context isn't 100% precise, but I also don't have a better idea (there we are again, naming things xD)

get_name function

It's as we discussed, asimple function that takes in a bufnr and returns the name for the buffer, you can pass in whatever function you would like that returns a string

printer function

Takes in snippets and returns a string. At the moment we are only using vim.inpect

win_opts

Table of extra options to be set on the window. set_win_opts sets these options.

I use these 2 to set the folding at the moment

buf_opts

Table of extra options to set on the buffer. set_buf_opts sets these options.

Good idea to make buf/win-options configurable

I use these 2 to set the filetype to lua. (I know you want something like nvim tree but in the meantime I kept this here just to get syntax highlighting)

Ahhh, yeah syntax highlighting makes sense.
If a different pretty-printer is used, one could write a custom syntax-file and use it by setting the filetype via buf-options, so that's pretty neat/complete.

make_scratch_buffer function

Just moved the setting options for the scratch buffer to this function to clean up the open_snippet_list function

So hopefully this is looking more like you were picturing in terms of configuration capability.

Discussion Points

Pretty Print Stuff

Similar to nvim-tree, but not of files and directories, but of snippets and file/snippet-types.

I think I get it now! Similar to what packer does when you sync/update plugins.

Yeeeeeeeeah right, that's even more common 👍

Gotcha. That would definitely be awesome, atm it's a little over my head though. I'm not sure how nvim-tree or packer make stuff collapsible and get highlighting/icons for example. I would have to go dig in their repos and see what they are doing.

Don't worry about that, this doesn't have to be perfect from the get-go. Updates to this can be done as non-breaking changes, so we're pretty flexible here imo.

In the meantime I left the folding method and the filetype to lua (For syntax highlighting purposes). With the configuration options we have now though it would be really easy to add whatever pretty-printer/buffer options/window options that you would like.

Very clean! I'll take a look at the code now :D

@L3MON4D3
Copy link
Owner

Okay, small things all, I like the general direction, nice work 👍

@ghost
Copy link
Author

ghost commented Nov 14, 2022

Nice! 👍
WDYT about renaming it? I think get_context isn't 100% precise, but I also don't have a better idea (there we are again, naming things xD)

😂 Hmm just brainstorming here get_snip_context, get_snip_tbl, get_snip_info, snip_context, snip_tbl, snip_info, snip_inspect, inspect_snip

@L3MON4D3
Copy link
Owner

MHMM snip_info sounds good, not perfect though xD
But I'd be fine with it

@ghost
Copy link
Author

ghost commented Nov 21, 2022

Sorry for the inactivity. Have been a little busier. I made the modifications suggested! Hopefully it's closer to what we want 😎

@L3MON4D3
Copy link
Owner

Oh, that's far from inactive, don't worry :D
From a cursory look, I like it, nice job!
I'll put some new comments in the code

@L3MON4D3
Copy link
Owner

Okay, I think this should be it, now.. Documentation :D
I think the existing entries for extras in DOC.md should be a good guideline, I'd recommend using grip to render the documentation locally (needs some setup, but is really nice for quickly iterating, as it renders markdown like github would)

@ghost
Copy link
Author

ghost commented Nov 29, 2022

Added some docs for Snippet List! Let me know what you think. Once everything is ready to go I can squash the previous commits having to do with implementing snippet_list so it looks like feat(extras): added snippet_list and then we're good to go :D

@L3MON4D3
Copy link
Owner

Hey, very sorry for not getting back earlier, I must've missed your previous messages somehow, and didn't think to check on this PR until now😅

@ghost
Copy link
Author

ghost commented Dec 20, 2022

Hey, very sorry for not getting back earlier, I must've missed your previous messages somehow, and didn't think to check on this PR until now😅

No worries! Thought you might be busy haha. I'll be travelling tomorrow but I'll take a look over the next few days see if we can finish this off before christmas or new years. We're almost there!

@L3MON4D3
Copy link
Owner

Nice, sounds good 👍 :D

@ghost
Copy link
Author

ghost commented Jan 6, 2023

Squashing of commits is done!

@ghost ghost marked this pull request as ready for review January 6, 2023 04:41
@L3MON4D3
Copy link
Owner

L3MON4D3 commented Jan 6, 2023

Okay I'm so sorry, I missed two minor issues:

  • pictures are embedded in vim-help, which is pretty ugly (fix: surround with ignore-tags, look ar source of gifs to see what I mean)
  • no doc for new capabilities of ls.available (I mean it's not that useful in itself, but there's no reason to not document it)

I'm taking another 10000%-close look to make sure those are the last roadblocks, would you clear them? :D

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Jan 6, 2023

Okay, 10000%-close look done, everything else looks great :)

@ghost
Copy link
Author

ghost commented Jan 6, 2023

Okay I'm so sorry, I missed two minor issues:

* pictures are embedded in vim-help, which is pretty ugly (fix: surround with ignore-tags, look ar source of gifs to see what I mean)

* no doc for new capabilities of `ls.available` (I mean it's not that useful in itself, but there's no reason to not document it)

I'm taking another 10000%-close look to make sure those are the last roadblocks, would you clear them? :D

Done!
Hopefully I didn't miss any pictures lol.
Checkout the updated description for the available function, I don't know if that's clear enough?

Edit: I'll squash those commits once it's approved.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Jan 6, 2023

Pictures look good, doc too except that small nit, thanks for taking care of it :)

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Jan 7, 2023

Haa okay, that's it, perfect, I've got nothing more to ask, you're free xD
One more squash, rebase if you wanna, then I'll merge and we've done it :)))))

@ghost
Copy link
Author

ghost commented Jan 7, 2023

Haa okay, that's it, perfect, I've got nothing more to ask, you're free xD One more squash, rebase if you wanna, then I'll merge and we've done it :)))))

Squash done!

Dobby

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Jan 8, 2023

Hah, nice xD

Okay, thank you sooo much for sticking through this entire process. I feel like I've asked a bit too much, and those small redesigns we did halfway through could've been separate PRs, but I'm very happy with this being the initial result because it's very flexible and we should be able to extend it without any breaking changes.
If you have any other ideas, feel free to open an issue to discuss it :)

@L3MON4D3 L3MON4D3 merged commit 508b41f into L3MON4D3:master Jan 8, 2023
@ghost
Copy link
Author

ghost commented Jan 8, 2023

Hah, nice xD

Okay, thank you sooo much for sticking through this entire process. I feel like I've asked a bit too much, and those small redesigns we did halfway through could've been separate PRs, but I'm very happy with this being the initial result because it's very flexible and we should be able to extend it without any breaking changes. If you have any other ideas, feel free to open an issue to discuss it :)

No problem! Glad I could contribute a small part to this plugin. Thank you for your work!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants