LuaSnip icon indicating copy to clipboard operation
LuaSnip copied to clipboard

feat: add fallback if no paths are currently loaded

Open pianocomposer321 opened this issue 3 years ago • 6 comments

This PR adds a fallback if no files are found in the current cache. This allows configs like the following:

require("luasnip.loaders.from_snipmate").lazy_load()

vim.api.nvim_create_user_command("LuaSnipEdit",
  function()
    require("luasnip.loaders").edit_snippet_files {
      fallback = function(ft, edit)
        if #ft > 0 then
          local filename = "~/.config/nvim/snippets/" .. ft .. ".snippets"
          print("Creating new file: " .. filename)
          edit(filename)
        else
          print("Canceling")
        end
      end
    }
  end, {})

Pretty minor change, should be self explanatory - basically it calls the fallback function, passing in the chosen filetype and the edit function. Main use case is to add file for current filetype if it does not already exist, but could be used for other things as well.

I made this because there was no easy way to tell if a file already exists and add it if it doesn't (at least not that I could tell). If I'm mistaken about this, I'd be interested in what the current preferred method is for this.

If I need to add docs or anything, just let me know!

pianocomposer321 avatar Jul 31 '22 03:07 pianocomposer321

Hey, thank you for participating, always happy to see some PRs from new ppl :)

Creating new snippet-files via :LuaSnipEdit is a great idea, should improve that workflow :+1:

One downside I can see with this implementation is that it is not possible to add a personal snippet-file if there already is one for this filetype, provided by some collection.

What do you think of, instead of fallback, providing a more general extend-callback, which gets a list of the files found by luasnip (ft_paths), and can return additional ones (as a list of {item_name, path}-pairs), which will then also be displayed in the second vim.ui.select-dialog?

For creating a new file, one could check the passed paths, if there is no item from the personal snippet-collection, an item for creating that file can be returned, maybe even with a hint that this selection creates a new file.

L3MON4D3 avatar Jul 31 '22 19:07 L3MON4D3

How does this look?

One thing I would say to note is that line 58 in loaders/init.lua will result in an error if the user-provided function does not return a table (e.g. it returns nil). This could be changed by adding ... or {} to the end of the line, but I thought it was probably best to leave it as is rather than force ignoring the type-error. But I'm open to your ideas about this.

pianocomposer321 avatar Aug 01 '22 00:08 pianocomposer321

BTW, this is an example of how I'm using it for reference:

vim.api.nvim_create_user_command("LuaSnipEdit", function()
  require("luasnip.loaders").edit_snippet_files {
    extend = function(ft, paths)
      if #paths == 0 then
        return {
          { "$CONFIG/" .. ft .. ".snippets",
            string.format("%s/%s.snippets", pc_path, ft) }
        }
      end

      return {}
    end
  }
end, {})

pianocomposer321 avatar Aug 01 '22 00:08 pianocomposer321

How does this look?

Looks great, I've no complaints :+1: Could you add a description of what extend does (and maybe an example of what it can be used for) here, in DOC.md (oh, and update that line stating that opts only contains one setting :sweat_smile:)?

One thing I would say to note is that line 58 in loaders/init.lua will result in an error if the user-provided function does not return a table (e.g. it returns nil). This could be changed by adding ... or {} to the end of the line, but I thought it was probably best to leave it as is rather than force ignoring the type-error. But I'm open to your ideas about this.

It's okay, as long as the doc specifies that extend should return a table. You could add an assert that extended actually is "table" though, directly point out misuse.

L3MON4D3 avatar Aug 01 '22 07:08 L3MON4D3

Sounds good. I won't be able to get to this for a while (probably at least a week and a half), just so you're aware why there isn't any progress.

pianocomposer321 avatar Aug 01 '22 13:08 pianocomposer321

Take your time :+1:

L3MON4D3 avatar Aug 01 '22 15:08 L3MON4D3

Hey, will you have time to finish this feature in the near future? (No worries if not, I'll take over and complete this, I think it's a nice addition)

L3MON4D3 avatar Sep 07 '22 17:09 L3MON4D3

Hey, will you have time to finish this feature in the near future? (No worries if not, I'll take over and complete this, I think it's a nice addition)

It's still my intention to get to it eventually, but I'm not sure when. I was on vacation in August, and life's been busy since getting back. TBH I haven't turned my workstation computer on since I got back.

So yes, but I'm not sure I can give you a timeline. I won't be offended if you just take over now, but I'm still willing to get to it eventually if you don't.

Completely up to you.

pianocomposer321 avatar Sep 07 '22 17:09 pianocomposer321

Ahh, alright :D I'm in no real hurry to get this in, so you can continue as soon as you find time for it (just wanted to make sure you're still up for finishing this :sweat_smile:)

L3MON4D3 avatar Sep 07 '22 18:09 L3MON4D3

Updated docs, added assert for extended. Is there anything else?

pianocomposer321 avatar Sep 15 '22 18:09 pianocomposer321

Ah nice :+1: I'll have look

L3MON4D3 avatar Sep 15 '22 19:09 L3MON4D3

How's this?

pianocomposer321 avatar Sep 18 '22 18:09 pianocomposer321

Very nice, great work :+1: Could you squash the commits?

L3MON4D3 avatar Sep 18 '22 19:09 L3MON4D3

(oh, and maybe rebase, that would be nice :D)

L3MON4D3 avatar Sep 18 '22 19:09 L3MON4D3

Lol, I don't actually know how to do that...I'm still learning how to use git properly. I will google it and get back with you.

Or you're welcome to tell me how to do it too, that way I know for sure I'm doing the right thing. Up to you.

pianocomposer321 avatar Sep 19 '22 18:09 pianocomposer321

I'm always squashing via git rebase -i HEAD~<numOfCommits> (important that the number isn't too big) and then interactively deciding to squash (maybe dropping the auto gen docs commits to avoid merge conflicts on the doc/luasnip.txt afterwards (your changes regarding the docs are in Doc.md and shouldn't get lost by that)).

Regarding the rebase on the master branch, I'd recommend to read on that (but from a users perspective it's not that different from a merge git rebase master -> fix the merge-conflicts).

Important: Just be aware that yo're rewriting the commit history with these methods (rebase). That means in order to push afterwards, you'll need a force push -> git -f push and that you might mess up the history, so maybe you want to make a backup of your local repo (the folder with .git in it) in advance just to be sure.

atticus-sullivan avatar Sep 19 '22 19:09 atticus-sullivan

(I'd do that stuff myself, but if I rebase, the commits would seem to be made by me, not you)

L3MON4D3 avatar Sep 19 '22 22:09 L3MON4D3

Ok, finally got around to this. Sorry for the wait (again 👀).

Did I do everything right? Is there anything else you need before merging?

pianocomposer321 avatar Oct 13 '22 20:10 pianocomposer321

Oh, I think something went wrong, theres some difference between this branch and master 250 commits ago, which should not be the case. I can take a look at this tomorrow

L3MON4D3 avatar Oct 13 '22 21:10 L3MON4D3

I think that the merge commit is breaking the history, it will be cleaner to remake the branch starting from the current master and cherry-pick the commit with the wanted changes only.

leiserfg avatar Oct 14 '22 08:10 leiserfg

That did it :+1: I also reworded the doc a bit, @pianocomposer321 if you're happy with that I'll go ahead and squash->merge

L3MON4D3 avatar Oct 14 '22 10:10 L3MON4D3

Looks good to me 👍

pianocomposer321 avatar Oct 14 '22 14:10 pianocomposer321

I think that the merge commit is breaking the history, it will be cleaner to remake the branch starting from the current master and cherry-pick the commit with the wanted changes only.

Also sorry about this. Glad you figured it out ;-).

pianocomposer321 avatar Oct 14 '22 14:10 pianocomposer321

Wonderful! Took a while, but we're there now :D Thank you!

L3MON4D3 avatar Oct 14 '22 18:10 L3MON4D3

Awesome! Sorry again for the long waiting periods and for the noobishness lol. Thanks for bearing with me...this was the first major project I've contributed to, and it was a great first experience. I learned a lot. Thanks again!

pianocomposer321 avatar Oct 14 '22 20:10 pianocomposer321

Oh wow, thank you, that's great to hear :D Don't worry about noobishness (and waiting periods especially, we're all doing this in our free time, some have more, some less), the actual code was pretty much perfect after your first iteration, it's just the organizational (documentation+git) stuff you were "noobish" at all in. I'm certain especially those will get easier with more experience :) Looking forward to more contributions from you, don't hesitate if you have an idea/are interested in helping out on issues

L3MON4D3 avatar Oct 14 '22 21:10 L3MON4D3