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: add fallback if no paths are currently loaded #502

Merged
merged 1 commit into from
Oct 14, 2022

Conversation

pianocomposer321
Copy link
Contributor

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!

@L3MON4D3
Copy link
Owner

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 👍

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.

@pianocomposer321
Copy link
Contributor Author

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
Copy link
Contributor Author

pianocomposer321 commented Aug 1, 2022

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, {})

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Aug 1, 2022

How does this look?

Looks great, I've no complaints 👍
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 😅)?

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.

@pianocomposer321
Copy link
Contributor Author

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.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Aug 1, 2022

Take your time 👍

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Sep 7, 2022

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)

@pianocomposer321
Copy link
Contributor Author

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.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Sep 7, 2022

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 😅)

@pianocomposer321
Copy link
Contributor Author

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

@L3MON4D3
Copy link
Owner

Ah nice 👍
I'll have look

pianocomposer321 added a commit to pianocomposer321/LuaSnip that referenced this pull request Sep 18, 2022
@pianocomposer321
Copy link
Contributor Author

How's this?

@L3MON4D3
Copy link
Owner

Very nice, great work 👍
Could you squash the commits?

@L3MON4D3
Copy link
Owner

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

@pianocomposer321
Copy link
Contributor Author

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.

@atticus-sullivan
Copy link
Contributor

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.

@L3MON4D3
Copy link
Owner

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

pianocomposer321 added a commit to pianocomposer321/LuaSnip that referenced this pull request Oct 13, 2022
Auto generate docs

feat: extend instead of fallback

Auto generate docs

fix: return empty table in default extend function

otherwise, if user doesn't specify an extend function, the default
returns nil, and we get an error when we try to loop through the result.

Format with stylua

Update documentation.

fix: assert type of exteded is table

fix: various problems mentioned by @L3MON4D3

See: L3MON4D3#502 (review)

Auto generate docs

Format with stylua

Auto generate docs

Format with stylua
@pianocomposer321
Copy link
Contributor Author

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

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

@L3MON4D3
Copy link
Owner

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

@leiserfg
Copy link
Contributor

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.

@L3MON4D3 L3MON4D3 force-pushed the master branch 2 times, most recently from 1c92701 to 20cb1c7 Compare October 14, 2022 10:12
@L3MON4D3
Copy link
Owner

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

@pianocomposer321
Copy link
Contributor Author

Looks good to me 👍

@pianocomposer321
Copy link
Contributor Author

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 ;-).

@L3MON4D3 L3MON4D3 merged commit 61238b9 into L3MON4D3:master Oct 14, 2022
@L3MON4D3
Copy link
Owner

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

@pianocomposer321
Copy link
Contributor Author

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!

@L3MON4D3
Copy link
Owner

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

# 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.

4 participants