Make Pisces Compatible with Fisher 4

[nickeb96]

Fisher was updated recently and no longer properly installs Pisces. Compatibility support for omf files like uninstall.fish no longer works. I moved the uninstall logic to the event system the way Fisher recommends. I also double checked to make sure the changes I made still work with older versions of Fisher, since many people may still have Fisher 3.

Btw, they changed the installation command again back to fisher install ... instead of fisher add ....

One more thing. I didn't check to see if these changes would work with omf. I don't use it and I don't know anything about it, but I noticed that Pisces still supports it. It may be as simple as re-adding the uninstall.fish file with just the following line:

emit pisces_key_bindings_uninstall

But I didn't include that in my commit. Someone who uses omf should double check to make sure that this would work.

[ngmoviedo]

I've installed @nickeb96 's fork via fisher and I can confirm that it works for me.

[jorgebucaran]

Left you a few comments @nickeb96.

[laughedelic]

hi guys! thanks for updating it. I would like to maintain compatibility with both omf and fisher if possible.
@nickeb96 since you mention trying it with omf, have you tried it on omf with these changes?

[jorgebucaran]

@laughedelic Fisher 4.x drops legacy init.fish and uninstall.fish file support. So, I suggest maintaining a dedicated OMF branch if you are so inclined.

[nickeb96]

@laughedelic I haven't used omf before, so I don't know if my suggestion would work for sure. It was just conjecture from reading their docs. I don't see a reason why it wouldn't work, but as jorgebucaran mentioned maintaining a separate branch is always an option.

[nickeb96]

Fixes #27

[jorgebucaran]

@nickeb96 ...re-adding the uninstall.fish file with just the following line...

You'll end up with an uninstall.fish file in your functions, so if another too-clever plugin tries to get away with the same trick to support OMF, they'll overwrite each other. Please don't do that.

[nickeb96]

@jorgebucaran that change isn't in the pull request, so it should still be ok to merge.

Does fisher now add all *.fish files in the repository root to ~/.config/fish/functions? I was under the impression that only files under repo-root/functions/ get moved to ~/.config/fish/functions.

[jorgebucaran]

@nickeb96 Yeah, I know, sorry. I just wanted to explain why the trick wouldn't work.

Does fisher now add all *.fish files in the repository root to ~/.config/fish/functions

Actually, this behavior was also in 3.x.

Don't use this feature, please. Put your functions inside a functions directory. Embarrassingly enough, I break this rule myself in Fisher, but only for historical reasons. I plan to migrate to functions/fisher.fish after we can declare 3x effectively dead and enough people have upgraded to 4.x or later.

[nickeb96]

@jorgebucaran Ah, that makes sense now. I never realized that was a thing, but hopefully it will eventually go away. Having functions confined to functions/ will be a bit cleaner and make plugin manager compatibility (i.e. omf) simpler.

[jorgebucaran]

On the contrary, that feature was introduced to support OMF plugins, e.g., bobthefish.

[patrickfatrick]

Just ran into this issue when installing. Look forward to seeing it merged.

[ngmoviedo]

I'm also looking forward to it! Meanwhile you can easily install @nickeb96 's fork via fisher install nickeb96/pisces or do the modifications manually.
But I hope it will be merged soon. I find this plugin really useful.

[patrickfatrick]

I do not believe this works as expected with OMF. it installs fine because OMF will execute the code in pisces.fish. However the uninstall code (and any code that runs on a fish event (install, update, uninstall)) is not be executed. I'm not sure how we can reconcile fisher and oh-my-fish at this point since fisher moves the root *.fish files to the functions directory and oh-my-fish needs those files for its own hooks.

[jorgebucaran]

@patrickfatrick We can't. See: #26 (comment)

[patrickfatrick]

@jorgebucaran Check out this PR I made for a package in a similar position: jhillyerd/plugin-git#49

What I wound up doing is:

• Removing init.fish and uninstall.fish from the root directory. The code in these files was moved to a couple of functions instead.
• Adding hooks/uninstall.fish which just calls the destroy function.
• Adding hooks/install.fish which just calls the init function.
• Adding conf.d/.fish which defines the fisher lifecycle hooks.

So now there are no root *.fish files, and the hooks directory contains the stuff for OMF. Since fisher does not care about the hooks directory nothing from there is copied over to the .config/fish directory.

[jorgebucaran]

Clever, but let me explain why I'm against attempting to do this on the same branch. Take this with a grain of salt, as I haven't really looked

• Maintaining a separate branch results in a cleaner codebase and makes it easier to support future framework-specific changes as well.
• Going out of our way to add OMF support ruins / goes against the simplicity of Fish canonical autorun mechanism that Fisher, etc. rely on.
• Creates more burden on plugin authors who need to maintain OMF specific-code on the same branch. A separate branch makes it easier to declare your interest in OMF support as well as remove it in the future if you stop caring about OMF. Not saying that you should, but you could.

[ngmoviedo]

I agree with @jorgebucaran , but maybe you should do your own PR and let @laughedelic pick the one he prefers or create two different branches, as @jorgebucaran suggested. I love the simplicity of Fisher and that's why I use it instead of OMF, but I guess some people may still prefer OMF, as it has some interesting features too.

[jorgebucaran]

@ngmoviedo Fair enough! It's ultimately up to the maintainer here to make that decision. Whatever you guys do, make sure to at least update the installation instructions, since fisher add is no longer a valid command. 😉

[patrickfatrick]

Totally, makes sense. At the very least I think this PR needs to indicate in the README that OMF is no longer supported given that it will not work with these changes. But given the state of OMF I don't know if it's worth continuing to support.

I'll go back to my PR and find out what the maintainer of that package would like to do, as well.

[nickeb96]

It's been a few weeks since making this. I'm hoping we can either merge this soon or add whatever is needed to merge it. @laughedelic, please let us know your thoughts.

[patrickfatrick]

@nickeb96 The uninstall hook here won't work with OMF so I think you either need to make it work (see #26 (comment)) or remove the OMF instructions from the readme so it's clear this package is only recommended for Fisher going forward. You can check out the PR linked in that comment for more details on how I got Fisher to work without breaking OMF. However, it's worth noting that the solution is a little bit of a hack and not in line with @jorgebucaran 's recommendations.

[laughedelic]

Hi everyone. Thanks for all the input. I didn't have time yet to dig into this and completely understand the whole incompatibility problem.

I appreciate the contribution @nickeb96, but I'm hesitant to merge this. I'll explain below why.

As far as I see from a quick search, the rumors about OMF's death have been greatly exaggerated: oh-my-fish/oh-my-fish#788. I don't understand why support for OMF plugins had to be removed in the new version of Fisher. I haven't updated my Fisher installation and I'm not planning to do it until I understand the consequences. As a user, I can't imagine what awesome features could motivate me to do that if I'd lose some plugins that just work now.

I'll make some comments on @jorgebucaran's points. Maybe I don't understand something, so please correct me if I'm wrong or overreacting, but the moment this is what I think:

Maintaining a separate branch results in a cleaner codebase (definitely less code) and makes it easier to support future changes as well.

I disagree with this. Maintaining different branches up to date is harder. When someone will contribute a feature or a fix, they will either have to make two separate PRs modifying different code or (more likely) they will care only about one package manager and keeping the other branch up to date will be on my shoulders. In my opinion maintaining one plugin that is compatible with two package managers is easier than maintaining two different plugins that attempt at the same functionality.

Going out of our way to add OMF support ruins the simplicity of Fish canonical autorun mechanism that Fisher relies on.

I really don't understand this. It looks reverse to me: my plugin did support both, but now with the Fisher update we have to go out of our way to support the latest Fisher and figure out this whole situation.

This creates more burden on plugin authors who need to maintain OMF specific-code on the same branch. A separate branch makes it easier to declare your interest in OMF support as well as remove it in the future if you stop caring about OMF.

It's not the inactivity in the OMF repo that's killing it, it's this: suggesting plugin authors to drop OMF support. I'm declaring my interest in maintaining OMF support until it's officially dead. I respect users choice of fish package manager and don't want to go along with the suggestion that it's just easier to support only one of them and thus contribute to killing the other one.

---

I'll leave this open for now. I'll look into this more when I have time or accept an alternative solution that supports both Fisher and OMF. So @patrickfatrick if your solution works as expected, I don't mind it being slightly less "clean" with respect to the new Fisher way.

[patrickfatrick]

@laughedelic I'll offer up my opinion as more of an outsider, being new to this whole fish thing (so take it with a grain of salt, I may be off-base).

It seems to me that the crux of the issue is that OMF promotes fish anti-patterns, eg the importance of init.fish, hooks, and key_bindings.fish: I'm new to the fish ecosystem so I don't know if OMF's design was or is addressing functionality missing in fish but, like, why an init.fish file in lieu of a conf.d/*.fish file which is essentially the same thing? Or why hooks files in lieu of fish events? Fisher appears to have its finger on pulse of functionality fish provides out of the box while OMF seems to be more about emulating oh-my-zsh (which is all about initializing scripts during startup).

Perhaps OMF is just "behind the curve" but if that's the case perhaps we should be clamoring for OMF to catch up, not asking Fisher to keep supporting outdated design patterns.

[jorgebucaran]

I don't understand why support for OMF plugins had to be removed in the new version of Fisher

It wasn't. We only dropped support for init.fish and uninstall.fish, thus still support OMF plugins that don't rely on those files. Why? They are obsolete. Fish has had a native/canonical way to autorun scripts since 2015-16 (fish-shell/fish-shell#2498) a.k.a configuration snippets (*.fish files under your $__fish_config_dir/conf.d/ directory), not to mention events, which have been available since forever.

...we have to go out of our way to support the latest Fisher

I'm sure @nickeb96 didn't mean to mislead anyone, but the title of this PR implies we need to go out of our way to support Fisher. We don't. Fisher didn't add anything new. What we want is for Pisces to leverage Fish configuration snippets, thus supporting any modern plugin manager or method of installation that uses them. There is nothing controversial about snippets, either. They are as old as the string builtin which this plugin heavily utilizes (both introduced in 2.3.0).

Maintaining different branches up to date is harder

I tend to disagree with my earlier point as well. Maintaining two branches can definitely be harder. I still promote a dual-branch approach because it's OMF that needs special treatment from Pisces. Keeping everything in the same codebase, while arguably harder to maintain, can be indirectly detrimental to the community as well. As a newcomer still figuring out how Fish works, I'd have a hard time trying to understand why things are so convoluted (which would be the case for a solution that supports both OMF-style and the Fish default autorun mechanism). However, it's ultimately up to the maintainer what to do here. Want to support both? Go for it.

It's not the inactivity in the OMF repo that's killing it, it's this: suggesting plugin authors to drop OMF support.

You're right, I shouldn't have suggested that outright. And it wasn't my place to do so either. I do suggest dropping support for obsolete Fish, and leveraging modern Fish, though.

I don't mind it being slightly less "clean" with respect to the new Fisher way.

There's no new Fisher way. It's the same 2015 way.

A great deal of this confusion comes from how we, and in particular, myself, have framed this discussion as a conflict between Fisher and OMF. But as it currently stands, it's Pisces that's going out of its way to support OMF. We've supported OMF obsolete autoload and uninstall mechanism for years. It's time to move on.

[nickeb96]

Hi again. I think I have re-enabled support for OMF. I originally proposed this change to Fisher which would have allowed my original suggestion to work, but I don't think it's going to be added. Instead I went with the suggestion from @patrickfatrick to use the hooks directory, which is specific to OMF. The uninstall hook I added just manually emits the uninstall event that would normally be emitted by Fisher automatically.

Someone who uses OMF should test this change to make sure it works. I followed the documentation and OMF example carefully so I don't see why it wouldn't, but I don't have OMF installed to test it out and be sure. I did check to make sure the change still works with Fisher and it does.

@laughedelic I think this is probably the simplest and most maintainable solution available. No separate branches or repositories are needed. No dropping of OMF support. Fisher 4 will now be supported. Fisher <4 will continue to be supported. And the code specific to each plugin manager is quite minimal.

[laughedelic]

@jorgebucaran I appreciate your thoughtful reply and clarifications! 🤝

@patrickfatrick

It seems to me that the crux of the issue is that OMF promotes fish anti-patterns, eg the importance of init.fish, hooks, and key_bindings.fish: I'm new to the fish ecosystem so I don't know if OMF's design was or is addressing functionality missing in fish

Yes, I think that was the case at the time. Quoting @jorgebucaran from #582:

I'd like to remove some OMF plugin support (it won't go away completely since more than less of OMF plugins actually work fine). But this means to fully deprecating init.fish, uninstall.fish, etc. See #581. The irony is that I introduced these conventions back in the day to OMF.

The irony is there indeed 😉 I understand, of course, that different times require different conventions and I'm all for innovation in Fisher. Innovation doesn't always have to be in direct contradiction with compatibility though. But that's up to the package manager maintainers to choose how to deal with it. I agree that OMF is "behind the curve", but it deserves a chance to adapt and at this point I think it's premature for plugin authors to drop support for it altogether.

@nickeb96 thanks for adapting the PR to support OMF! I tried it out and it seems to work as expected, including uninstallation 💯