search

gekoke / magit-file-icons

File icons for Magit
GNU General Public License v3.0
30 stars 6 forks source link

feat: add all-the-icons support #5

Closed librephoenix closed 2 months ago

librephoenix commented 2 months ago

This PR adds all-the-icons support via a new variable magit-file-icons-icon-backend. This defaults to 'nerd-icons, but is automatically set to 'all-the-icons as a fallback if nerd-icons isn't installed. It also abstracts the icon getter functions into magit-file-icons-icon-for-file-func and magit-file-icons-icon-for-dir-func which can be set as custom function aliases if magit-file-icons-icon-backend is set to nil (in order to potentially support icon backends other than nerd-icons or all-the-icons).

I made just a few mistakes as I was debugging so there's some unneeded reversions in some of the commits. If you'd like me to redo them to be more clean (or fix any other part of it), I'd be happy to.

Also, this is my first PR ever! I hope its helpful! Thanks for making this package!

gekoke commented 2 months ago

Hi! Thanks a lot!

I made just a few mistakes as I was debugging so there's some unneeded reversions in some of the commits. If you'd like me to redo them to be more clean (or fix any other part of it), I'd be happy to.

No worries, I'll probably squash merge anyway.

Just a heads up, you might have to rebase onto main after #7 is merged, as it does modify some of the patching code that had to change due to recent updates in magit.

Also, this is my first PR ever!

Congrats!

gekoke commented 2 months ago

On a different note, you may have noticed that I follow the conventional commits commit message convention for this project. In the future, it's best to conform to the commit message conventions of any project you contribute to (including in the PR title, as this is used to infer commit messages in various GitHub merge operations).

gekoke commented 2 months ago

Without even delving into the changes themselves, we might want to consider dropping the dependency on nerd-icons now that we will support more than one backend, and leave it to the users to provide whichever backend they want.

librephoenix commented 2 months ago

Thanks for all of the targeted feedback! It was very helpful to learn from!

I decided to just make a new branch called abstract-icon-getters based on the updated main branch and I think the new implementation should be much simpler. I'll close this PR and open a new one with the updated chages, and you'll have to let me know what you think!

Thanks!