search

pyenv / pyenv-virtualenv

a pyenv plugin to manage virtualenv (a.k.a. python-virtualenv)
MIT License
6.3k stars 398 forks source link

fix: PATH update for non-fish shells #474

Open keikoro opened 8 months ago

keikoro commented 8 months ago

Reloading my shell config leads to duplication of path segments related to pyenv-virtualenv because the command:

eval "$(pyenv virtualenv-init -)"

which is part of my config, as per the README, is run without any checks of the current PATH being done in the background.

I've been experiencing this problem for many years but never had the time to look into it until now. Was surprised to not find any issues pointing out this problem, especially since the code has been around (unchanged) for so long, but found PR #430, in which another user fixed this problem for the fish shell a couple of years ago.

My PR looks to do the same for the catch-all case statement which follows the fish shell check. I'm not familiar with the code construct the other user used, so went with a statement that works for me in my zsh config. I'll note that I didn't test this in shells other than zsh, and also don't know enough about other shells and how they deal with bash scripts to be able to tell if this could not work in others.

keikoro commented 8 months ago

Looks to me like the tests are meant to duplicate the original code but with the URL ${TMP}/pyenv/plugins/pyenv-virtualenv/shims, so that's what I did for the failing tests.

native-api commented 8 months ago

Note an important detail in both https://github.com/pyenv/pyenv-virtualenv/pull/430 and https://github.com/pyenv/pyenv/blob/e676fde9909946fc0078ec921936eb079c24fd8d/libexec/pyenv-init#L146-L147 : not only does it deduplicate PATH entries, it also pushes the entry to the start of PATH. Practice has shown that this is what users typically want when an .rc file is executed again (e.g. exec $SHELL, nested interactive shell e.g. in Vim). For rare advanced cases where this is not what the user wants, we've added a --no-push-path switch in Pyenv.

keikoro commented 8 months ago

Ok. what does this mean in concrete terms for this PR? That it should reuse the existing code?

As said, I didn't fully understand what it was doing – which comes at no surprise, I guess, if it actually does more than just check for a string (which I, personally, wouldn't consider a feature, but ok) – so I didn't reuse it.

Sidenote: The README of this repo doesn't make mention of this flag (ETA: and nor does pyenv's README), are there plans to make users aware of it?

native-api commented 8 months ago

Ok. what does this mean in concrete terms for this PR?

The PR should likewise push the PATH entry to the beginning of PATH.