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.

Sign up for GitHub

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

Jump to bottom

Fix for the removal of all the environments using the --all option #8665

Closed
wants to merge 6 commits into from
Closed

Fix for the removal of all the environments using the --all option #8665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9606bbe
Fixed env remove all command
laclouis5 Nov 13, 2023
ab08473
Fixed tests
laclouis5 Nov 13, 2023
2071558
Reformatting
laclouis5 Nov 13, 2023
9755fee
Merge branch 'master' into env-remove-all-fix
laclouis5 Nov 18, 2023
2376761
Fixed wrong python number
laclouis5 Nov 25, 2023
f83f949
Merge branch 'master' into env-remove-all-fix
laclouis5 Dec 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/poetry/console/commands/env/remove.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def handle(self) -> int:
self.line(f"Deleted virtualenv: <comment>{venv.path}</comment>")
if all:
for venv in manager.list():
manager.remove_venv(venv.path)
manager.remove_venv(venv)
self.line(f"Deleted virtualenv: <comment>{venv.path}</comment>")

return 0
36 changes: 30 additions & 6 deletions src/poetry/utils/env/env_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ def remove(self, python: str) -> Env:
if venv.path.name == python:
# Exact virtualenv name
if not envs_file.exists():
self.remove_venv(venv.path)
self.remove_venv_dir(venv.path)

return venv

Expand All @@ -348,15 +348,15 @@ def remove(self, python: str) -> Env:

current_env = envs.get(base_env_name)
if not current_env:
self.remove_venv(venv.path)
self.remove_venv_dir(venv.path)

return venv

if current_env["minor"] == venv_minor:
del envs[base_env_name]
envs_file.write(envs)

self.remove_venv(venv.path)
self.remove_venv_dir(venv.path)

return venv

Expand Down Expand Up @@ -405,10 +405,34 @@ def remove(self, python: str) -> Env:
del envs[base_env_name]
envs_file.write(envs)

self.remove_venv(venv_path)
self.remove_venv_dir(venv_path)

return VirtualEnv(venv_path, venv_path)

def remove_venv(self, venv: VirtualEnv) -> Env:
venv_path = self._poetry.config.virtualenvs_path
cwd = self._poetry.file.path.parent
envs_file = TOMLFile(venv_path / self.ENVS_FILE)
base_env_name = self.generate_env_name(self._poetry.package.name, str(cwd))

env_path = venv.path
major, minor, _ = venv.get_version_info()

name = f"{base_env_name}-py{major}.{minor}"

if not env_path.exists():
raise ValueError(f'<warning>Environment "{name}" does not exist.</warning>')
Comment on lines +423 to +424
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this warning? At least raising a ValueError seems to be wrong, especially for a warning.

Copy link
Author

Choose a reason for hiding this comment

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

ValueError is the error type that was chosen for this kind of warning I guess. I sticked to the behavior of the original .remove() method which uses ValueError in this exact same way.


if envs_file.exists():
envs = envs_file.read()
if base_env_name in envs:
del envs[base_env_name]
envs_file.write(envs)
Comment on lines +428 to +430
Copy link
Member

Choose a reason for hiding this comment

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

It may not make a difference for remove --all but considering that this method removes exactly one venv, we should only remove the entry if the Python version matches the version of the venv we are removing.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed, this may no make a difference. The check is only there if there is an inconsistency between the TOML env file and the virtual env folders.


self.remove_venv_dir(env_path)

return VirtualEnv(env_path, env_path)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we return a venv?

Copy link
Author

Choose a reason for hiding this comment

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

Again, I sticked to what appears to be the norm in the Poetry repository. The original .remove() method returns a virtual env, thus I kept that design parttern here too.


def use_in_project_venv(self) -> bool:
in_project: bool | None = self._poetry.config.get("virtualenvs.in-project")
if in_project is not None:
Expand Down Expand Up @@ -576,7 +600,7 @@ def create_venv(
self._io.write_error_line(
f"Recreating virtualenv <c1>{name}</> in {venv!s}"
)
self.remove_venv(venv)
self.remove_venv_dir(venv)
create_venv = True
elif self._io.is_very_verbose():
self._io.write_error_line(f"Virtualenv <c1>{name}</> already exists.")
Expand Down Expand Up @@ -684,7 +708,7 @@ def build_venv(
return cli_result

@classmethod
def remove_venv(cls, path: Path) -> None:
def remove_venv_dir(cls, path: Path) -> None:
assert path.is_dir()
try:
remove_directory(path)
Expand Down
6 changes: 4 additions & 2 deletions tests/utils/test_env.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,8 @@ def test_activate_activates_recreates_for_different_patch(
"poetry.utils.env.EnvManager.build_venv", side_effect=build_venv
)
remove_venv_m = mocker.patch(
"poetry.utils.env.EnvManager.remove_venv", side_effect=EnvManager.remove_venv
"poetry.utils.env.EnvManager.remove_venv_dir",
side_effect=EnvManager.remove_venv_dir,
)

env = manager.activate("python3.7")
Expand Down Expand Up @@ -516,7 +517,8 @@ def test_activate_does_not_recreate_when_switching_minor(
"poetry.utils.env.EnvManager.build_venv", side_effect=build_venv
)
remove_venv_m = mocker.patch(
"poetry.utils.env.EnvManager.remove_venv", side_effect=EnvManager.remove_venv
"poetry.utils.env.EnvManager.remove_venv",
side_effect=EnvManager.remove_venv_dir,
)

env = manager.activate("python3.6")
Expand Down