PatrickF1 / fzf.fish

🔍🐟 Fzf plugin for Fish
MIT License
1.96k stars 78 forks source link

Add support for FZF_DEFAULT_OPTS_FILE #319

Closed stdlo closed 5 months ago

stdlo commented 5 months ago

Don't set default fzf options if FZF_DEFAULT_OPTS_FILE is set. FZF_DEFAULT_OPTS_FILE was added to fzf in version 0.47.0

PatrickF1 commented 5 months ago

Hi, thank you for the PR! I will take a look sometime this weekend or next week. And just so you know, we'll need to update the readme too.

Out of curiosity so I can empathize with my users better, what makes you want to the FZF_DEFAULT_OPTS_FILE file over the env var? Is it for uniformity across shells?

PatrickF1 commented 5 months ago

Hmm I can't push to your repo.

Could you give me edit permissions on your fork

OR

Can you update this section of the readme with this:

### Change fzf options for all commands

fzf supports global default options via the [FZF_DEFAULT_OPTS or FZF_DEFAULT_OPTS_FILE](https://github.com/junegunn/fzf#environment-variables) environment variables.

`fzf.fish` sets [a sane `FZF_DEFAULT_OPTS` whenever it executes fzf](functions/_fzf_wrapper.fish) unless you export your own `FZF_DEFAULT_OPTS` or `FZF_DEFAULT_OPTS_FILE`.

?

stdlo commented 5 months ago

Apologies for the slow responses! I've added you as a collaborator, as well as made the changes you requested myself.

As for why I wanted this, fzf recently added support for the options file and on my initial setup I was using that. I've since started using the env just because it seems better supported across plugins right now. Here's a link to the release that enabled the option file if you want some additional details: https://github.com/junegunn/fzf/releases/tag/0.47.0

PatrickF1 commented 5 months ago

No worries, open source work is slow. I'll get this merged sometime this weekend cause I'm busy as well.

Thanks for explaining! Yeah I think I'll move over to using FZF_DEFAULT_OPTS_FILE myself to reduce work maintaining the opts across each shell and ability to change opts without reloading shell.

it seems better supported across plugins right now Sorry one more question, which plugins better support it right now?

stdlo commented 5 months ago

Sorry one more question, which plugins better support it right now?

This is more of an assumption, the feature is new as of like 3 weeks ago.

Although fzf directly reads FZF_DEFAULT_OPTS _FILE. So I guess it would only impact plugins that do what fzf.fish does and read/write the FZF_DEFAULT_OPTS variable.

PatrickF1 commented 5 months ago

Oh I see. I misread

I've since started using the env just because it seems better supported across plugins right now.

I thought you meant you started using the options file because plugins support it better but you meant you switched to the env variable because plugins better support it. I agree it'll take time for plugins like this to adapt.

PatrickF1 commented 5 months ago

Sorry for the delay. I'm still working on why the tests are failing on Ubuntu. I use Mac so it's hard for me to debug

PatrickF1 commented 5 months ago

Hmm I do not know why I couldn't push to your branch's main even after you gave me edit access. And force pushing closed this PR. Let me try to fix this...

PatrickF1 commented 5 months ago

Thanks for your contribution!