-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Add virtualenv prompt for robbyrussell theme #2060
base: master
Are you sure you want to change the base?
Conversation
function prompt_command() { | ||
PS1="${bold_green}➜ ${bold_cyan}\W${reset_color}$(scm_prompt_info)${normal} " | ||
PS1="$(venv_prompt)${bold_green}➜ ${bold_cyan}\W${reset_color}$(scm_prompt_info)${normal} " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a little too opinionated to place the venv_prompt even before the ➜
... not sure if before scm_prompt_info or after it is better, but I'm leaning toward after ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, venv_prompt was usually on the leftmost side of the P1 by default with virtualenv, pipenv, and pyenv. robbyrussell
theme from oh-my-zsh implements it also on the leftmost side before the ➜
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidpfarrell Is there another way to modify the prompt of the theme, without changing the original theme? Because, original theme is not showing virtualenv prompt, can it be overriden through the .bashrc
? Or leave somehow up to the underlying venv to modify prompt with their default (but theme seems to just ignores venv modifications of PS1 - don't know why).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petarnikolovski, as it stands now most themes overwrite $PS1
before displaying every prompt. Modifications to $PS1
are just entirely lost immediately after setting them. Something like the PowerLine theme(s) allow for a lot more fine tuning, but that requires using one of those themes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidpfarrell I've checked virtualenv activate script, and it places the prompt on the leftmost side:
mkdir /tmp/project
cd /tmp/project
python3 -m venv env
cat env/bin/activate
...
if [ -z "${VIRTUAL_ENV_DISABLE_PROMPT:-}" ] ; then
_OLD_VIRTUAL_PS1="${PS1:-}"
if [ "x(env) " != x ] ; then
PS1="(env) ${PS1:-}"
else
if [ "`basename \"$VIRTUAL_ENV\"`" = "__" ] ; then
# special case for Aspen magic directories
# see http://www.zetadev.com/software/aspen/
PS1="[`basename \`dirname \"$VIRTUAL_ENV\"\``] $PS1"
else
PS1="(`basename \"$VIRTUAL_ENV\"`)$PS1"
fi
fi
export PS1
fi
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greetings and thanks for submitting a PR !
I have a comment regarding placement of the new prompt - feels a bit out of place at the very start of the prompt line ... My suggestion would be to place it after the scm prompt segment ...
818f095
to
1ec8f9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we already have a variation on venv_prompt
! It would be better to use, or enhance, those rather than create a duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! 😃
@davidpfarrell @gaelicWizard any update on this, is it ready to be merged? |
function conda_or_venv_prompt() { | ||
local python_venv="" | ||
if [[ -n "${CONDA_DEFAULT_ENV:-}" ]]; then | ||
python_venv=$(condaenv_prompt) | ||
PYTHON_VENV_CHAR=${CONDA_PYTHON_VENV_CHAR} | ||
elif [[ -n "${VIRTUAL_ENV:-}" ]]; then | ||
python_venv=$(virtualenv_prompt) | ||
fi | ||
[[ -n "${python_venv}" ]] && echo "${PYTHON_VENV_CHAR}${python_venv}" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should probably live closer to the existing functions.
NOTE: PYTHON_VENV_CHAR
and CONDA_PYTHON_VENV_CHAR
are only used by the themes that do not invoke the virtualenv_prompt
or condaenv_prompt
functions. Those function use PYTHON_THEME_PROMPT_PREFIX
and PYTHON_THEME_PROMPT_SUFFIX
The spirit of the methods that live in this base theme is to use the PREFIX
and SUFFIX
variables.
I think this function just becomes:
if [[ -n "${CONDA_DEFAULT_ENV:-}" ]]; then
condaenv_prompt
elif [[ -n "${VIRTUAL_ENV:-}" ]]; then
virtualenv_prompt
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @petarnikolovski, want to continue and improve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NoahGorny Sorry for the late response, I will make moves on improvement soon, I couldn't manage because of my work schedule.
Hey there @petarnikolovski , do you want to finish this PR, or merge it as is? |
@seefood Hi, I'm swamped with work and cannot continue working on this. I'm ok with the merge as is. Otherwise, we can close this. |
@petarnikolovski ok, I'll circle back to it some day, I want to make sure I understood the affect correctly. |
Description
Added virtualenv prompt for robbyrussell theme (same as in the purity theme)
Motivation and Context
Usually when one
cd
into directory with active virtualenv name of the virtualenv is displayed on the left hand side in parentheses.How Has This Been Tested?
cd into and out of the directory with virtualenv has desired effect
Screenshots (if appropriate):
https://imgur.com/D0ZG5Nc
Types of changes
Checklist:
clean_files.txt
and formatted it usinglint_clean_files.sh
.