Skip to content

Suppress activate/deactivate messages by default #169

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

Merged
merged 7 commits into from
Apr 19, 2016
Merged

Suppress activate/deactivate messages by default #169

merged 7 commits into from
Apr 19, 2016

Conversation

puhitaku
Copy link
Contributor

@puhitaku puhitaku commented Apr 8, 2016

This PR includes changes that suppress message on virtualenv activation and de-activation.
Detailed info is here: #168

What I did

Added if statement around echoes in bin/pyenv-sh-activate and bin/pyenv-sh-deactivate. If users want pyenv to show verbose activate/deactivate message, they have to configure export PYENV_VIRTUALENV_DISABLE_PROMPT=1.

@puhitaku
Copy link
Contributor Author

puhitaku commented Apr 8, 2016

I've forgot adding appropriate tests. Please wait a little ...

@@ -138,7 +138,9 @@ fi

pyenv-sh-deactivate --force --quiet ${VERBOSE+--verbose} || true

echo "pyenv-virtualenv: activate ${venv}" 1>&2
if [ -n "$PYENV_VIRTUALENV_VERBOSE_ACTIVATE" ]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you think how we should manage existing ${VERBOSE} here? In my guts feeling, this line should care about both ${VERBOSE} and ${PYENV_VIRTUALENV_VERBOSE_ACTIVATE} too....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean like if [ -n "$PYENV_VIRTUALENV_VERBOSE_ACTIVATE" -a -n "$VERBOSE" ] ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERBOSE seems to be used with the called tools only currently?! (python/pip)

I assume the main this about this PR is to change this after all?!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically the ${VERBOSE} is used mainly from internally. I think just checking ${PYENV_VIRTUALENV_VERBOSE_ACTIVATE} here could be sufficient at least for now.

@yyuu
Copy link
Collaborator

yyuu commented Apr 11, 2016

CI's still failing. I'll try to look into this once I get a chance....

@puhitaku
Copy link
Contributor Author

Sorry for being late but bats' test drives me crazy. I've edited test codes but it doesn't work as I expected (bats says that expected output and actual output are different, but they look same for me.) I'll push (still not working) additional commits later.

blueyed added a commit to blueyed/pyenv-virtualenv that referenced this pull request Apr 12, 2016
This is used by `pyenv activate` and is unnecessarily noisy (and
confusing).

This seems to be sensible regardless of having an "activate" message
with or without `--verbose`
(see pyenv#169).
@blueyed
Copy link
Collaborator

blueyed commented Apr 12, 2016

@puhitaku
For the tests, you might want to push what you have (and squash it later), so it can be reviewed.
See #170 btw ("deactivate" message with pyenv activate and no active virtualenv (because of --force)), which I've came across a while ago myself.

@puhitaku
Copy link
Contributor Author

Yes! It's passing now.

@blueyed
Copy link
Collaborator

blueyed commented Apr 13, 2016

LGTM (from the behavior).
I would like for PYENV_VIRTUALENV_DISABLE_PROMPT=0 to behave logically (i.e. as if unset) though.
So it should check for != 0 or = 1 instead. But feel free to ignore this.

@@ -32,7 +32,6 @@ setup() {
assert_success
assert_output <<EOS
deactivated
pyenv-virtualenv: activate anaconda-2.3.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unset PYENV_VIRTUALENV_VERBOSE_ACTIVATE in the setup of this file.

@yyuu yyuu merged commit b952f57 into pyenv:master Apr 19, 2016
@puhitaku
Copy link
Contributor Author

Oh I've forgot to tell that I've pushed commits.
Anyway thanks for merging!

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.

3 participants