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

Add Poetry support to virtualenv #2450

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

alexjurkiewicz
Copy link
Contributor

Integrate Poetry virtualenv detection into the existing virtualenv segment. For poetry virtualenvs, use the package name as reported by poetry version.

Fixes #1994.

This PR isn't working quite right. I'll add review comments where I need help from someone more experience 🙏

Heavily based on romkatv#1994.

Integrate Poetry virtualenv detection into the existing virtualenv
segment. For poetry virtualenvs, use the package name as reported by
`poetry version`.
internal/p10k.zsh Outdated Show resolved Hide resolved
internal/p10k.zsh Outdated Show resolved Hide resolved
Added for debugging purposes.
Don't use echo, instead set a variable which the calling function
checks. This is the standard in p10, presumably because it's faster..?
Make them a little less ambiguous.
We were comparing virtualenv's name to pyenv's version, which would
never be the same.

Now this setting works as intended, although I'm not sure how useful it
is. It's probably more useful to reorder this logic so virtualenv has
precedence.
Pre-Poetry support, we only entered this codepath when we were inside a
virtualenv. Now we enter it for every directory, but we don't want to
display an empty segment for non-virtualenv locations.
Comment on lines +4315 to +4325
_virtualenv_name=''
if [[ -n $VIRTUAL_ENV ]]; then
_virtualenv_load_name_VIRTUAL_ENV
else
local start end
_p9k_upglob pyproject.toml
local idx=$?
if (( idx > 0 )); then
_virtualenv_load_name_poetry $idx
fi
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice if this code had the structure

_virtualenv_name=''
if in_virtual_env; then
  load_name_from_env_vars
elif in_poetry_project; then
  load_poetry_project_name
fi

The current structure is functionally the same, but harder to reason about.

Here's an alternate implementation. I'm not sure if it's better or worse, what do you think?

_virtualenv_load_name_from_env_vars || _virtualenv_load_name_from_poetry || _virtualenv_name=''

_virtualenv_load_name_from_env_vars() {
  [[ -z ${VIRTUAL_ENV} ]] && return 1
  _virtualenv_name=${VIRTUAL_ENV}
}
_virtualenv_load_name_from_poetry() {
  _p9k_upglob pyproject.toml || return 1
  _p9k_cached_cmd 0 "$pyproject" poetry -C "$dir" version
  _virtualenv_name="${_p9k__ret%% *}"
}

Or do you have a better idea?

@alexjurkiewicz
Copy link
Contributor Author

Future improvement. Currently there's logic to show virtualenv segment if the current Python version differs from pyenv's. Personally I would like the opposite: show pyenv only if there's no active virtualenv.

@romkatv
Copy link
Owner

romkatv commented Oct 15, 2023

#1994 (comment) applies here as well.

I'll keep the PR open in case I do find time to learn poetry and implement support for it in powerlevel10k. At that point your changes might be useful.

@alexjurkiewicz
Copy link
Contributor Author

alexjurkiewicz commented Oct 16, 2023 via email

@romkatv
Copy link
Owner

romkatv commented Oct 16, 2023

Sure, thanks. Is there anything I can do to help? 🙏

Either I need to learn poetry and implement support for it in powerlevel10k (this is how I've developed all other prompt segments), or someone needs to send a PR that I can merge with no changes. The latter can happen not only when the PR is 100% correct in terms of functionality and style, but also when it's obvious to me that it is indeed 100% correct (not an easy thing to achieve given that I never used poetry and don't know what it does).

You have two options that don't rely on me:

  1. Implement the segment you need as a custom segment.
  2. Fork powerlevel10k and implement your changes invasively.

(1) will be easier for you to maintain.

@alexjurkiewicz
Copy link
Contributor Author

alexjurkiewicz commented Oct 17, 2023 via email

@romkatv
Copy link
Owner

romkatv commented Oct 17, 2023

Just to clarify what you mean by "custom segment". Do you mean creating a new segment called e.g. "poetry" which I submit as a pull request?

I won't merge a PR unless it's obviously correct and matches the style of the surrounding code perfectly. This is a ridiculously high bar. In practice this means that a non-trivial PR has virtually no chance of getting merged.

This is why I suggested that instead of sending PRs you implement a custom prompt segment for yourself using the public and documented API provided by powerlevel10k. See p10k help segment and search for "example" in ~/.p10k.zsh. This will allow you to implement the segment(s) you need without depending on my merging anything.

@alexjurkiewicz
Copy link
Contributor Author

Got it. I'll leave this PR as-is and use something custom locally 👌

@android10
Copy link

android10 commented Mar 5, 2024

@alexjurkiewicz if you want to benefit from the current venv functionality from @romkatv, you can set up poetry to create virtual environments in the project directory, thus, your prompt will show you this information as if you were using pip.

I use this approach for python project compatibility and full isolation, although of course that is my opinion :).

You can achieve what I mentioned above by either setting an ENV variable:

export POETRY_VIRTUALENVS_IN_PROJECT=true

or by having poetry global config in your $XDG_CONFIG_HOME/pypoetry/config.toml.
Here is mine for example:

virtualenvs.in-project = true
virtualenvs.prefer-active-python = true
virtualenvs.options.always-copy = true

Then you can activate your virtual environment and it will be display the right information:

$ source .venv/bin/activate

or

$ poetry shell

Check the Poetry Official Documentation for more.

I hope that helps.

@romkatv
Copy link
Owner

romkatv commented Mar 5, 2024

Thanks for posting this tip!

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