search

jorgebucaran / fisher

A plugin manager for Fish
https://git.io/fisher
MIT License
7.55k stars 257 forks source link

Fisher 4.x release notes don't mention dropping support for key_bindings.fish #678

Closed lilyball closed 3 years ago

lilyball commented 3 years ago

The Fisher 4.0.0 release notes mentions deprecating init.fish and uninstall.fish, but it makes no mention of key_bindings.fish. Similarly, the Fisher 4.2.0 release notes says "Remove support for .fish files outside a functions directory".

What's more, Fisher 4.0.0 dropped the init.fish, uninstall.fish, and key_bindings.fish support without emitting any warnings, and instead of simply ignoring the files, it started copying them into the functions/ dir instead. This isn't "deprecating" them, this is breaking them entirely and causing unwanted behavior. It wasn't until Fisher 4.2.0 when support for files outside of subdirs was dropped that we got warnings, and Fisher 4.3.0 removed the warnings anyway.

Which is to say, I have two plugins of mine that just have a top-level key_bindings.fish file and I had no idea they were broken, because Fisher orphaned the old conf.d/$plugin.key_bindings.fish files so they continued to work in my setup. But I also ended up with a functions/key_bindings.fish file that I didn't realize (which is a copy of the file from whichever of the two plugins was installed last). Even when updating to Fisher 4.2.0 I didn't realize they were broken, because Fisher doesn't re-invoke its own updated version to do an install and I had no reason to re-run fisher install until updating to Fisher 4.3.0, at which point I thankfully got the warning during that install (but re-running it no longer shows that warning, I just have to notice that it didn't list any installed files).

All of this to say, the documentation for Fisher 4.x never mentioned that a top-level key_bindings.fish was no longer supported (and the upgrade was botched as described above in terms of Fisher 4.0/4.1 installing this into the functions dir). Which made it very confusing for me to figure out what's going on. It's too late to fix the botched upgrade, but here's what you can do:

Also I think the broken plugin warning should be brought back for plugins that don't install any files. This could be because it has only top-level fish files, or it could be because the repo isn't a fish plugin at all. This wouldn't be "Support for .fish files outside a functions directory is deprecated", but instead emit something like the following:

fisher: Plugin not supported: lilyball/alt-q.fish
        Note: top-level .fish files are no longer supported
fisher: Plugin not supported: lilyball/not-a-fish-plugin
        This does not appear to be a fish plugin
Installing lilyball/alt-q.fish
Installing lilyball/not-a-fish-plugin
Installing jorgebucaran/fisher
           /Users/lily/.config/fish/functions/fisher.fish
           /Users/lily/.config/fish/completions/fisher.fish

If the plugin has at least one file to install then no warning would be emitted.

jorgebucaran commented 3 years ago

Thank you, @lilyball. The language was definitely incorrect. I went ahead and updated the release notes accordingly.

Also I think the broken plugin warning should be brought back for plugins that don't install any files.

Who would benefit from this and why are people still creating Fish plugins with top-level .fish files?

lilyball commented 3 years ago

Who would benefit from this and why are people still creating Fish plugins with top-level .fish files?

It wouldn’t be for people creating new plugins, it would be for people trying to install older plugins that haven’t been updated for Fisher 4.x.

lilyball commented 3 years ago

In fact, the README still advertises “ Oh My Fish! plugin support” but this isn’t true anymore. The top-level init/uninstall/key_bindings files comes from that and they aren’t supported anymore. I haven’t gone looking at existing OMF plugins but any that rely on this support will be broken.

jorgebucaran commented 3 years ago

https://github.com/jorgebucaran/fisher/issues/651 actually prompted the OMF maintainers to update plugins that had files living outside the prescribed directories, and even migrate init files to configuration snippets! 😄