Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for FZF_DEFAULT_OPTS_FILE #319

Merged
merged 4 commits into from Apr 7, 2024
Merged

Conversation

stdlo
Copy link
Contributor

@stdlo stdlo commented Mar 26, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

stdlo commented Apr 2, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

stdlo commented Apr 2, 2024

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
Copy link
Owner

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
Copy link
Owner

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

# See https://github.com/junegunn/fzf#environment-variables
if not set --query FZF_DEFAULT_OPTS
if not set --query FZF_DEFAULT_OPTS || not set --query FZF_DEFAULT_OPTS_FILE
Copy link
Owner

@PatrickF1 PatrickF1 Apr 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I think this is wrong. We will enter this branch if either FZF_DEFAULT_OPTS or FZF_DEFAULT_OPTS_FILE are not set, even if the other one IS set. It only skips the branch if both are set. I'll fix it. This is probably why the Linux tests are failing.

@PatrickF1
Copy link
Owner

PatrickF1 commented Apr 7, 2024

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 PatrickF1 reopened this Apr 7, 2024
@PatrickF1 PatrickF1 merged commit 8920367 into PatrickF1:main Apr 7, 2024
5 checks passed
@PatrickF1
Copy link
Owner

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants