laloch / xontrib-fzf-widgets

Set of fzf widgets for xonsh
GNU General Public License v3.0
33 stars 18 forks source link

Unexpected handling of FZF_DEFAULT_COMMAND #9

Open wshanks opened 4 years ago

wshanks commented 4 years ago

The way FZF_DEFAULT_COMMAND is handled here is a little odd to me:

https://github.com/laloch/xontrib-fzf-widgets/blob/62aacd796c96b9ac9682b57d165105419c162c8d/xontrib/fzf-widgets.xsh#L74-L83

For one thing, I think env = dict(os.environ) would be better because I think that the point here is to change the value of FZF_DEFAULT_COMMAND just for the single subprocess? The way it works now os.environ is permanently changed, though os.environ is kind of weird in Python -- not everything uses it. Its values are not linked to the values in ${...} for example. Even better than env = dict(os.environ) would be env = ${...}.detype() (unless I am missing some reason to favor os.environ over ${...}).

What prompted me to open this issue is that my FZF_DEFAULT_COMMAND generated a command with color codes. I tried changing it in my .xonshrc but the change was not taking effect because I was in a xonsh subshell where the parent still had my old FZF_DEFAULT_COMMAND set. Since os.environ gets set when os is imported and doesn't get further updates, my settings in .xonshrc were not applied to os.environ.

So to summarize, I am mainly suggesting a one line change to use env = ${...}.detype() but perhaps there is a different way to address the issue I was encountering.

laloch commented 4 years ago

Yeah, you are right @willsALMANJ. I think the best would be something like

with ${...}.swap(FZF_DEFAULT_COMMAND=fzf_find_command) as env:
    denv = env.detype()
    choice = subprocess.run(..., env=denv)

Or maybe even better would be to make sure to always run the fzf executable unthreaded and to get rid of the subprocess.run(...) call altogether. It's IMO doable in Xonsh.

I never really took the time to clean the xontrib up after I had adopted it. I'll try to get to it soon. To be completely honest, I rather spend my time improving Xonsh itself than playing with this petty project when it does actually work quite fine for me.