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

Ensure variables set by conf.d/fzf.fish are global and update uninstaller #122

Merged
merged 3 commits into from
Feb 10, 2021
Merged

Ensure variables set by conf.d/fzf.fish are global and update uninstaller #122

merged 3 commits into from
Feb 10, 2021

Conversation

etcusrvar
Copy link
Contributor

I believe you want this to be global.
If fisher_path is undefined and this file is in $__fish_config_dir/conf.d, it will already be global by default.
If fisher_path is defined, and $fisher_path/conf.d has to be sourced manually, it will default to local, which I believe is not intended.

@PatrickF1
Copy link
Owner

Hi @etcusrvar, thanks for contributing a bug fix! I'm afraid I don't understand. What does fisher_path have to do with this?

@etcusrvar
Copy link
Contributor Author

I probably should have tested this a bit more before opening this issue.
TL;DR - it's a local setup issue-- but explicitly setting global may still be good idea.

In my local setup, I essentially follow this example-- but not exactly.
Instead of

for file in $fisher_path/conf.d/*.fish
    source $file
end

I have

function __source_fisher_confd
    for file in $fisher_path/conf.d/*.fish
        source $file
    end
end
__source_fisher_confd

In the first case, the default scope when sourcing conf.d/fzf.fish is global, but in the second case, being wrapped in a function, the default scope is local.

I have a few other fisher plugins installed that also set variables, but they explicitly set globals where appropriate, so I only saw this issue with your plugin. Even though the issue is with my setup, you may still want to explicitly use --global for FZF_DEFAULT_OPTS. (And __fzf_search_vars_cmd, which I can add to this PR if desired.)

@PatrickF1
Copy link
Owner

Ohhh I see....I finally understand what the set --help docs mean:

If a variable is not explicitly set to be either universal, global or local and has never before been
defined, the variable will be local to the currently executing function. Note that this is different from
using the -l or --local flag. If one of those flags is used, the variable will be local to the most inner
currently executing block, while without these the variable will be local to the function. If no function is
executing, the variable will be global.

Yes, making those explicitly global is a great idea. Thanks @etcusrvar !

@PatrickF1 PatrickF1 changed the title fix: make FZF_DEFAULT_OPTS global Ensure variables set by conf.d/fzf.fish are global and update uninstaller Feb 10, 2021
conf.d/fzf.fish Show resolved Hide resolved
@PatrickF1 PatrickF1 merged commit 8a299b4 into PatrickF1:main Feb 10, 2021
@etcusrvar
Copy link
Contributor Author

etcusrvar commented Feb 10, 2021

Cool. Thanks! (Great extension, by the way.)

@etcusrvar etcusrvar deleted the make-env-var-global branch February 10, 2021 16:26
hrshtst pushed a commit to hrshtst/fzf.fish that referenced this pull request Apr 22, 2021
…ller (PatrickF1#122)

Explicitly declare the variables with the --global flag because if they get sourced from a function, they will be locally scoped to the function rather than globally scoped and therefore not picked up by fzf functions. This can happen if the user is using customizing fisher by using fisher_path and has to source their conf.d files themselves.
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.

2 participants