Skip to content

Comments

Update env remove logic#6195

Merged
neersighted merged 5 commits intopython-poetry:masterfrom
dhvcc:bug/6018
Sep 17, 2022
Merged

Update env remove logic#6195
neersighted merged 5 commits intopython-poetry:masterfrom
dhvcc:bug/6018

Conversation

@dhvcc
Copy link
Contributor

@dhvcc dhvcc commented Aug 18, 2022 •
edited
Loading

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
@dhvcc
Removed hardcoded project names for env testing
04b0a92 
Added venv_name fixture to tests/utils
@dhvcc
Update full path handling for env remove
4d91dad 
Add IncorrectEnvError exception
Add check_env_is_for_current_project method
Add a couple of tests for new and old env remove logic
@dhvcc
Update env remove logic
2700195 
Update remove handling, so that if other project's venv name is passed, raise IncorrectEnvError for better error message
@dhvcc
Merge branch 'master' into bug/6018
a8aa448
@dhvcc dhvcc changed the title Update env remove logic [WIP] Update env remove logic Aug 19, 2022
@neersighted
Copy link
Member

neersighted commented Sep 17, 2022

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

neersighted commented Sep 17, 2022

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

neersighted commented Sep 18, 2022

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

@radoering radoering mentioned this pull request Dec 3, 2022
Merged
@github-actions
Copy link

github-actions bot commented Feb 29, 2024

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.

Reviewers

@neersighted neersighted neersighted approved these changes

Assignees

No one assigned

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

Milestone

1.3

Development

Successfully merging this pull request may close these issues.

poetry env remove removes wrong environment

3 participants

@dhvcc @neersighted @Secrus