search

sorin-ionescu / prezto

The configuration framework for Zsh
MIT License
13.98k stars 4.49k forks source link

Support `virtualenvwrapper` with / without `pyenv` `virtualenv-init` or `virtualenvwrapper` plugins #2023

Closed jeffwidman closed 1 year ago

jeffwidman commented 1 year ago

The desired logic is:

For the pyenv plugins virtualenv-init and virtualenvwrapper:

  1. If either plugin is present, activate it
  2. If virtualenvwrapper plugin is not present, then fallback to standard virtualenvwrapper.
  3. If virtualenvwrapper plugin is present, then don't fallback to standard virtualenvwrapper, regardless of whether virtualenv-init is present.

Previously, if the virtualenv command was present but pyenv was missing, then the fallback wouldn't be hit. This bug was introduced by https://github.com/sorin-ionescu/prezto/pull/1981/ which ensured that the pyenv virtualenvwrapper plugin was activated if present, regardless of the presence of the virtualenv-init plugin.

As an optimization, the check for the pyenv plugins are skipped if pyenv itself isn't found.

Since we only want to fallback if the pyenv virtualenvwrapper plugin is missing, but that's buried within the pyenv logic and we also need to handle when pyenv itself is missing, this switches to using a flag variable.

I also renamed the virtualenv_sources var to virtualenvwrapper_sources as virtualenv is distinct from virtualenvwrapper, so using one name for a var that is really about the other is confusing.

Looking at git blame, there's a lot of prior art here around trying to support all the permutations of pyenv and various plugins:

So we need to be extremely careful to continue to support all these permutations.

Fix https://github.com/sorin-ionescu/prezto/issues/2022

jeffwidman commented 1 year ago

@zbirenbaum @diraol @zachwhaley since you were all involved in prior PR's around this section of code logic, can you take this for a test drive to ensure I'm not accidentally breaking something in your use cases with this.

Also @indrajitr / @belak I'd appreciate some sanity check reviews.

zbirenbaum commented 1 year ago

I like this too. My change was a quick one to prevent people from getting locked out of their systems since that’s a really bad bug to have. This is a better formalization.

jeffwidman commented 1 year ago

Thanks all!

I'm going to merge given the positive reception. If we find further bugs/unhandled edge cases, I'm happy to take another look.