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

gh-89427: Provide the original prompt value for VIRTUAL_ENV_PROMPT #106726

Merged

Conversation

jimporter
Copy link
Contributor

@jimporter jimporter commented Jul 13, 2023

This improves the implementation in gh-106643.

Previously, venv passed "(<prompt>) " to the activation scripts, but we want to provide the original value so that users can inspect it in the $VIRTUAL_ENV_PROMPT env var.

Note: Lib/venv/scripts/common/Activate.ps1 surrounded the prompt value with parens a second time, so no change was necessary in that file.


Sorry for the noise; I didn't quite understand the flow of this code until I started comparing it more closely to the implementation in the virtualenv package.

I was careful to test this change in action (only using the Bash version of the activation script, since that's the shell I have), but I also checked the generated scripts and they all look correct to me. After calling python -m venv foobar (results are also as expected if I pass --prompt=something):

activate

VIRTUAL_ENV_PROMPT="foobar"
export VIRTUAL_ENV_PROMPT

# ...

if [ -z "${VIRTUAL_ENV_DISABLE_PROMPT:-}" ] ; then
    _OLD_VIRTUAL_PS1="${PS1:-}"
    PS1="(foobar) ${PS1:-}"
    export PS1
fi

activate.csh

setenv VIRTUAL_ENV_PROMPT "foobar"


set _OLD_VIRTUAL_PROMPT="$prompt"

if (! "$?VIRTUAL_ENV_DISABLE_PROMPT") then
    set prompt = "(foobar) $prompt"
endif

activate.fish

set -gx VIRTUAL_ENV_PROMPT "foobar"

# ...

if test -z "$VIRTUAL_ENV_DISABLE_PROMPT"
    # ...
    function fish_prompt
        # ...
        # Output the venv prompt; color taken from the blue of the Python logo.
        printf "(%s%s%s) " (set_color 4B8BBE) "foobar" (set_color normal)
        # ...
    end

    set -gx _OLD_FISH_PROMPT_OVERRIDE "$VIRTUAL_ENV"
end

@jimporter jimporter requested a review from vsajip as a code owner July 13, 2023 17:43
@jimporter jimporter force-pushed the gh-89427/no-parens-around-virtual-env-prompt branch from 858a810 to 41800a3 Compare July 13, 2023 18:11
@jimporter jimporter marked this pull request as draft July 13, 2023 18:21
@jimporter jimporter force-pushed the gh-89427/no-parens-around-virtual-env-prompt branch from 41800a3 to d65e6ee Compare July 13, 2023 19:14
@jimporter jimporter marked this pull request as ready for review July 13, 2023 19:31
@jimporter jimporter force-pushed the gh-89427/no-parens-around-virtual-env-prompt branch 2 times, most recently from f1fbcfd to e676da1 Compare August 4, 2023 02:52
@jimporter
Copy link
Contributor Author

@vsajip Is there anything else I should do with this patch before it merges? This change will keep venv's behavior in-sync with virtualenv (see pypa/virtualenv#2606).

@vsajip
Copy link
Member

vsajip commented Aug 4, 2023

Sorry, not had time to review this. Hope to get to it soon.

@jimporter
Copy link
Contributor Author

Sorry, not had time to review this. Hope to get to it soon.

No problem! Just wanted to make sure you weren't waiting on me for something.

@jimporter jimporter force-pushed the gh-89427/no-parens-around-virtual-env-prompt branch from e676da1 to 980d15d Compare November 16, 2023 01:23
@jimporter
Copy link
Contributor Author

@vsajip I rebased this and fixed the merge conflicts, which had the nice benefit of actually making the diff smaller.

This improves the implementation in gh-106643.

Previously, venv passed "(<prompt>) " to the activation scripts, but we want
to provide the original value so that users can inspect it in the
$VIRTUAL_ENV_PROMPT env var.

Note: Lib/venv/scripts/common/Activate.ps1 surrounded the prompt value with
parens a second time, so no change was necessary in that file.
@jimporter jimporter force-pushed the gh-89427/no-parens-around-virtual-env-prompt branch from 980d15d to d658e85 Compare January 14, 2024 22:10
@jimporter
Copy link
Contributor Author

@vsajip Just checking again to see if you have bandwidth to review. I rebased again so it should merge cleanly.

@vsajip vsajip merged commit 8edc802 into python:main Jan 23, 2024
30 checks passed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…MPT (pythonGH-106726)

This improves the implementation in pythongh-106643.

Previously, venv passed "(<prompt>) " to the activation scripts, but we want
to provide the original value so that users can inspect it in the
$VIRTUAL_ENV_PROMPT env var.

Note: Lib/venv/scripts/common/Activate.ps1 surrounded the prompt value with
parens a second time, so no change was necessary in that file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants