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

[python] Edit pyenv virtualenv-init check to support non-brew installations #1433

Merged
merged 1 commit into from Aug 18, 2017

Conversation

@zachwhaley
Copy link
Contributor

@zachwhaley zachwhaley commented Aug 17, 2017

Fixes #1428

Proposed Changes

If the pyenv virtualenv plugin is installed using the pyenv-installer
app script or directly via a Git clone, then the pyenv-virtualenv-init
executable, that the pyenv virtualenv-init command uses, will not
exist in the user's PATH and therefore cannot be found using $commands[pyenv-virtualenv-init].
Installing the pyenv-virtualenv plugin in this manner is common among Linux users.

Using the pyenv commands command, which lists all commands pyenv can
run, we can find if the virtualenv-init command is available to pyenv
and subsequently the virtualenv plugin, without relying on
pyenv-virtualenv-init to exist in the user's PATH.

If the pyenv virtualenv plugin is installed using the [pyenv-installer][1]
app script or directly via a [Git clone][2], then the pyenv-virtualenv-init
executable, that the `pyenv virtualenv-init` command uses, will not
exist in the user's PATH and therefore cannot be found using
`$commands[pyenv-virtualenv-init]`.
Installing the pyenv-virtualenv plugin in this manner is common among Linux users.

Using the pyenv `commands` command, which lists all commands pyenv can
run, we can find if the virtualenv-init command is available to pyenv
and subsequently the virtualenv plugin, without relying on
pyenv-virtualenv-init to exist in the user's PATH.

[1]: https://github.com/pyenv/pyenv-installer
[2]: https://github.com/pyenv/pyenv-virtualenv#installing-as-a-pyenv-plugin
if (( $+commands[pyenv] && $+commands[pyenv-virtualenv-init] )); then
if (( $+commands[pyenv] )) && \
pyenv commands | command grep -q virtualenv-init
then
eval "$(pyenv virtualenv-init -)"
# Optionall activate 'virtualenvwrapper' with 'pyenv' is available.
if (( $#commands[(i)pyenv-virtualenvwrapper(_lazy|)] )); then

This comment has been minimized.

@diraol

diraol Aug 17, 2017
Contributor

Shouldn't the same type of check from line 97 be done here? (pyenv-virtuanenvwrapper)

This comment has been minimized.

@zachwhaley

zachwhaley Aug 17, 2017
Author Contributor

I believe so, but I don't have that plugin installed and I didn't want to overcomplicate this PR.

If you'd like me to look into this, I can install virtualenvwrapper and give it a go.

This comment has been minimized.

@indrajitr

indrajitr Aug 17, 2017
Collaborator

Yes, I think so. Essentially, we can cannot assume pyenv plugins to be present in $PATH in all cases. We'll have to revisit the treatment for pyenv plugin detection in general.

This comment has been minimized.

@zachwhaley

zachwhaley Aug 17, 2017
Author Contributor

@indrajitr Sorry, I don't think I understood. Yes, I should make the same change to pyenv-virtualenvwrapper?

And

we can assume pyenv plugins to be present in $PATH

Did you mean cannot, because that is the reason for this change.

This comment has been minimized.

@indrajitr

indrajitr Aug 17, 2017
Collaborator

You got me :)

This comment has been minimized.

@zachwhaley

zachwhaley Aug 17, 2017
Author Contributor

So after installing virtualenvwrapper and making those changes, I'm left wondering what all the conditionals on line 88 are testing for. Escpecially zstyle -T ':prezto:module:python:virtualenv' initialize, which seems to be used to set whether the user wants to initialize virtualenvwrapper or not.

In my case, I would not, but I would want to enable virtualenv.

So I'm wondering if this block of code needs a little reworking to allow for virtualenv without initializing virtualenvwrapper?

This comment has been minimized.

@zachwhaley

zachwhaley Aug 18, 2017
Author Contributor

I'm gonna walk myself back on this since it seems like a safe assumption that if you installed the virtualenvwrapper plugin, you probably want to use it.

if (( $+commands[pyenv] && $+commands[pyenv-virtualenv-init] )); then
if (( $+commands[pyenv] )) && \
pyenv commands | command grep -q virtualenv-init
then
eval "$(pyenv virtualenv-init -)"
# Optionall activate 'virtualenvwrapper' with 'pyenv' is available.

This comment has been minimized.

@belak

belak Aug 17, 2017
Collaborator

Unrelated issue - this should be "Optionally"

This comment has been minimized.

@indrajitr

indrajitr Aug 17, 2017
Collaborator

And s/with/when/ :)

@@ -93,7 +93,9 @@ if (( $+VIRTUALENVWRAPPER_VIRTUALENV || $+commands[virtualenv] )) && \
VIRTUAL_ENV_DISABLE_PROMPT=1

# Enable 'virtualenv' with 'pyenv'.
if (( $+commands[pyenv] && $+commands[pyenv-virtualenv-init] )); then
if (( $+commands[pyenv] )) && \
pyenv commands | command grep -q virtualenv-init

This comment has been minimized.

@belak

belak Aug 17, 2017
Collaborator

This should probably be a check for [[ -n $(pyenv commands | command grep -q virtualenv-init) ]] (https://google.github.io/styleguide/shell.xml?showone=Testing_Strings#Testing_Strings) and the then should be on the same line as the conditional (https://google.github.io/styleguide/shell.xml?showone=Loops#Loops)

This comment has been minimized.

@zachwhaley

zachwhaley Aug 17, 2017
Author Contributor

The first line is not checking a string, but the return code of the command.

I'll fix the then conditional.

This comment has been minimized.

@belak

belak Aug 17, 2017
Collaborator

You're right. My mistake. I wasn't paying attention to the -q... also pipelines in if statements usually trip me up. I tend to think it's the first command in the pipeline, but it's actually the last... one of the reasons I try to avoid pipes in conditionals if possible is that it's easy to mess that up.

@indrajitr
Copy link
Collaborator

@indrajitr indrajitr commented Aug 18, 2017

Personally, I am not a fan of | and grep noises since these can be mostly achieved directly using zsh.
That said, I am going ahead and merging the PR because this is a decent attempt at fixing the initial problem (which is an obvious bug to me) with awesome analysis in commit log.

I'll make a followup PR little later with relevant tweaks.

@indrajitr indrajitr merged commit 3194442 into sorin-ionescu:master Aug 18, 2017
@gpanders
Copy link
Contributor

@gpanders gpanders commented Aug 18, 2017

One other change I'd like to suggest: the check for the ':prezto:module:python:virtualenv' initialize zstyle seems to be wrong. Currently, it is using

zstyle -T ':prezto:module:python:virtualenv' initialize

This is returning true even if the relevant line in .zpreztorc is commented out. It should be

zstyle -t ':prezto:module:python:virtualenv' initialize 'yes'

But this PR just got merged, so does this need to be a new PR now?

@indrajitr
Copy link
Collaborator

@indrajitr indrajitr commented Aug 18, 2017

And thanks @zachwhaley for the first PR!

@belak
Copy link
Collaborator

@belak belak commented Aug 18, 2017

@gpanders It's a bit confusing, but the values in zpreztorc are supposed to be the defaults, even when commented out, so I believe this is expected behavior. Thanks for mentioning it though!

@gpanders
Copy link
Contributor

@gpanders gpanders commented Aug 18, 2017

@belak Interesting. So where is that value set, then, if not in zpreztorc?

@indrajitr
Copy link
Collaborator

@indrajitr indrajitr commented Aug 18, 2017

As inlined doc says, it defaults to yes and thus enabled.

So to disable, you'll have to explicitly specify no in ~/.zpreztorc:

zstyle ':prezto:module:python:virtualenv' initialize 'no'

This has been done to stay compatible with previous behavior (when it wasn't backed by a zstyle setting).

@zachwhaley zachwhaley deleted the zachwhaley:pyenv-virtualenv branch Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.