cvigilv / esqueleto.nvim

Reduce your boilerplate code the lazy-bones way
MIT License
100 stars 7 forks source link

Provide option to ignore certain files #26

Closed Futarimiti closed 1 year ago

Futarimiti commented 1 year ago

Hi, thanks for the helpful plugin!

I believe it is necessary for certain files to not be picked up as templates. An infamous example is .DS_Store:

Select skeleton to use:
1: .DS_Store
2: default

To address this, I added a new option ignore, a function that, given the filepath, determines if a file should be ignored. Here's its default implementation:

ignore = function (f) return vim.fn.fnamemodify(f, ':t') == '.DS_Store' end

Please let me know if this looks good to you. :)

cvigilv commented 1 year ago

Thanks for the PR!

Will check the code more in-depth whenever I have a minute. Said that, I don't really know how this would fit the package since it's seems to be a per-user "problem" (I also use MacOS and haven't had this problem).

Could you please show me your config and some more in-depth use cases of this PR to see if it makes sense to add?

Thanks in advance!

Futarimiti commented 1 year ago

Thanks for the reply. Let me explain:

So .DS_Store files are created by Finder in every single accessed folder, basically everywhere in the macOS filesystem - which is reflected in numerous repos' .gitignores. Just try open your skeletons directory with Finder, go in and out different skeleton directories a few times, and .DS_Store should populate soon. They will then be inadvertently recognised as a skeleton templates because function gettemplates indiscriminately pick up every file within the skeleton directories. This can interrupt the smooth experience of autouse for filetypes with solely a default skeleton. As far as I know, there are no existing ways to filter unwanted files unless you delete them manually.

My proposal is to add a filter mechanism, namely ignore, to allow users to specify which files are not be regarded as templates. Users may also designate discarded skeletons or comment files after this addition, which is viable but not the primary purpose of this feature.

...since it's seems to be a per-user "problem".

Well, therefore instead of hardcoding files users may not desire, I made it a "per-user" option that users can specify their own file filter, for example Windows users may add desktop.ini to the list. Speaking of that, it should be technically better to implement ignore with a list of filename patterns instead of a function, but I'm having a hard time with lua patterns so currently I'll just toss the hot potato to users. Would be happy to know if anyone could improve on that though.

cvigilv commented 1 year ago

Now I get it, this is really a general problem no matter what your OS is. The PR is a little bit convoluted to achieve what you intend, therefore I'll refactor it and do the following in order to merge the PR:

Thanks for the explanation! This will be a nice QoL improvement for users.

Futarimiti commented 1 year ago

Mostly done I think. I also added another option use_os_ignore where users can choose to turn off the builtin os filter and use their own entirely, if they'd like to.

Futarimiti commented 1 year ago

@cvigilv ready for review now. I'll add documentation later if you think it looks good.

cvigilv commented 1 year ago

Im reviewing your PR an I have a couple of questions/comments:

  1. You implemented a new function getunignoredtemplates, which is basically gettemplates but with the filter. Why didn't you just extend the latter? I'll remove this one and rename yours to gettemplates in order to abstract the work done under the hood.
  2. In my opinion, the PR implements something I would consider and "advanced" user feature. I'll refactor your PR to encompass this behavior as an advanced config and place the ignore table and use_os_ignore option inside.
  3. I'll rename the use_os_ignore to ignore_os_files
  4. Documentation related to the functionality of this PR has to be written once I add the things I mentioned before.

Lastly but most importantly: Thanks for the work! This will be an awesome quality of life upgrade to the plugin

cvigilv commented 1 year ago

Cleaned up the commit history a little bit so when we merge the pull request its clearer what is happening.

You are welcomed to test the changes I made and write the documentation for this feature, so we can finish with this request.

Thanks again!

Futarimiti commented 1 year ago

Looks all good to me. Just updated README with the new options.

Futarimiti commented 1 year ago

Forgot to mention that ignore could also be a function. Should be good right now.

cvigilv commented 1 year ago

Please add documentation to docstrings so one can access this information using :h esqueleto. With that done, I'll merge the PR.

Futarimiti commented 1 year ago

Done.

cvigilv commented 1 year ago

Merged! Thanks for your contribution!