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

fix: PATH update for non-fish shells #474

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keikoro
Copy link
Contributor

@keikoro keikoro commented Jan 20, 2024

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.

Checks PATH variable for existing inclusion of shims
for shells other than fish.

See PR pyenv#430
@keikoro
Copy link
Contributor Author

keikoro commented Jan 20, 2024

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

native-api commented Jan 20, 2024

Note an important detail in both #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
Copy link
Contributor Author

keikoro commented Jan 20, 2024

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

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

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

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