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

Update env remove logic #6195

Merged
merged 5 commits into from
Sep 17, 2022
Merged

Update env remove logic #6195

merged 5 commits into from
Sep 17, 2022

Conversation

dhvcc
Copy link
Contributor

@dhvcc dhvcc commented Aug 18, 2022

Pull Request Check List

Resolves: #6018

  • Added tests for changed code.
  • Updated documentation for changed code. (no documentation change is needed I think)

Changes

  1. Added a check so that if python argument is a file (then it should be a python path) - extract it's venv name and raise IncorrectEnvError if it doesn't belong to this project
    Before
    └❯ poetry env remove ~/.cache/pypoetry/virtualenvs/different-project-OKfJHH_5-py3.10/bin/python
    /bin/sh: 1: different-project-OKfJHH_5-py3.10: not found
    
    Deleted virtualenv: ~/.cache/pypoetry/virtualenvs/poetry-4pWfmigs-py3.10
    
    Removes current project's env, which is wrong.
    After
    └❯ poetry env remove ~/.cache/pypoetry/virtualenvs/different-project-OKfJHH_5-py3.10/bin/python
    
    Env different-project-OKfJHH_5-py3.10 doesn't belong to this project.
    
  2. Added the exact same check as before ^, but for cases where env name is passed.
    Before
    └❯ poetry env remove different-project-OKfJHH_5-py3.10      
     /bin/sh: 1: different-project-OKfJHH_5-py3.10: not found
    
     Command different-project-OKfJHH_5-py3.10 -c "import sys; print('.'.join([str(s) for s in sys.version_info[:3]]))" errored with the following return code 127, and output: 
    
    Errors while trying to exec env name as an interpreter, error is not clear.
    After
    └❯ poetry env remove different-project-OKfJHH_5-py3.10                      
    
    Env different-project-OKfJHH_5-py3.10 doesn't belong to this project.
    
  3. Added a couple of tests for new and for old scenarios which weren't tested.
  4. Added venv_name fixture for tests/utils directory to use in test_env. Also replaced some of "simple-project" hardcoded value to use poetry.package.name

It's up to maintainers to choose what they want for this project - I'm happy either way if we at least fix the bug. I can remove/change any of the stuff I added on top of the fix. But yeah I just decided that if we fix the bug, we might also make some improvements/changes in this area of code. Any thoughts on this are welcome, thanks!

dhvcc added 4 commits August 18, 2022 21:11
Added venv_name fixture to tests/utils
Add IncorrectEnvError exception
Add check_env_is_for_current_project method
Add a couple of tests for new and old env remove logic
Update remove handling, so that if other project's venv name is passed, raise IncorrectEnvError for better error message
@dhvcc dhvcc changed the title Update env remove logic [WIP] Update env remove logic Aug 19, 2022
@neersighted
Copy link
Member

I think this code could use a general refactor/cleanup to make it clear we now support 'identifiers' that can be Python version numbers, Python executable paths, or environment names (as well as maybe a docs pass). If you'd like to do that, it would be great -- if not, this is still incremental progress.

@neersighted neersighted added this to the 1.3 milestone Sep 17, 2022
@neersighted neersighted added area/venv Related to virtualenv management kind/enhancement Not a bug or feature, but improves usability or performance impact/changelog Requires a changelog entry labels Sep 17, 2022
@neersighted neersighted merged commit 2def357 into python-poetry:master Sep 17, 2022
@dhvcc
Copy link
Contributor Author

dhvcc commented Sep 17, 2022

@neersighted yeah, I agree this could be a bit clearer in the code. I'd like to try to clean this up, wondering if there's any plan/ideas for it right now?

@neersighted
Copy link
Member

Personally I would rework the code to actually be a simple env.remove() (actually using self to find the environment to remove) and then add functionality to EnvManager to find/create an Env instance for the identifier in question.

@neersighted
Copy link
Member

If you're interested in working in this area still, #2748 might provide some more ideas for improvements to the CLI experience.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/venv Related to virtualenv management impact/changelog Requires a changelog entry kind/enhancement Not a bug or feature, but improves usability or performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

poetry env remove removes wrong environment
3 participants