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

[laclouis5]

Resolves: #8631

• Added tests for changed code.
• Updated documentation for changed code.

I didn't added new tests because one is already here for this command (there is a test for poetry env remove --all) and don't think new documentation is required since this is a simple bug fix. I ran all the tests and linting/formatting tools listed in the contribution guidelines before submitting this PR.

The previous code did't remove the TOML sections in envs.toml when removing all virtual environment using poetry env remove --all. I implemented a new method on the EnvManager for this specific case. I also renamed the previous method EnvManager.remove_venv to the more explicit name EnvManager.remove_venv_dir and updated the references.

I struggled a little to understand if the environments listed in envs.toml should be kept in sync with the virtual environments present in the parent folder. I supposed the negative since I think that this assertion is never checked in the tests, thus I haven't modified the tests to check this.

Feel free to suggest improvements.

[dimbleby]

I never use poetry env and I'm not too sure what envs.toml is used for in all honesty. Could you simply have deleted that file from the "remove all" branch?

well this looks plausible too, hopefully someone who remembers something about this code will take a look

[laclouis5]

Looks like it is used pervasively through the code dealing with envs to me. I don't think it should be deleted.

[dimbleby]

I meant: would an alternative fix have been to remove the file at the same time as removing all environments? I assume the world starts off with no environments and no envs.toml so I guess that's a valid state.

But it doesn't matter anyway, you've taken a different approach and as I say that looks plausible to me.

[laclouis5]

Ah, there is a misunderstanding I think! The command poetry env remove --all does not remove all the existing Poetry environments but only all the environments associated with the current project.

[radoering]

I didn't added new tests because one is already here for this command (there is a test for poetry env remove --all)

I don't follow this argument. If I understand correctly, this PR fixes a bug. In that case, there should be a test that fails without the fix. If there isn't such a test, then it should be added. (It shouldn't be too difficult to create an envs.toml in a test and check that the concerning section is removed.)

[radoering]

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

[laclouis5]

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.

[radoering]

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.

[laclouis5]

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.

[radoering]

Why do we return a venv?

[laclouis5]

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.

[laclouis5]

I didn't added new tests because one is already here for this command (there is a test for poetry env remove --all)

I don't follow this argument. If I understand correctly, this PR fixes a bug. In that case, there should be a test that fails without the fix. If there isn't such a test, then it should be added. (It shouldn't be too difficult to create an envs.toml in a test and check that the concerning section is removed.)

I wasn't able to tell if consistency between the TOML file and the env folders has to be strictly enforced since no test for the remove command seems to validate this assertion. I was simply conservative and preferred not to change the tests. The TOML could be out of sync with the env folders for many reasons, for instance a user manually removing them from the cache because they take too much space on their device.

I checked the tests for the create command (triggered by env use) and it appears that some kind of consistency is checked. The new environment should be listed in the TOML but the rest of the TOML isn't checked for consistency.

I you confirm that doing basic consistency checks makes sense, i'll considerer adding tests for that.

[radoering]

Ah, now I understand: the new method is just a partial copy of the existing remove method, to be more precise it's more or less

poetry/src/poetry/utils/env/env_manager.py

Lines 339 to 359 in 0578ed8

	# Exact virtualenv name
	if not envs_file.exists():
	self.remove_venv(venv.path)
	return venv
	venv_minor = ".".join(str(v) for v in venv.version_info[:2])
	base_env_name = self.generate_env_name(cwd.name, str(cwd))
	envs = envs_file.read()
	current_env = envs.get(base_env_name)
	if not current_env:
	self.remove_venv(venv.path)
	return venv
	if current_env["minor"] == venv_minor:
	del envs[base_env_name]
	envs_file.write(envs)
	self.remove_venv(venv.path)

In that case, we should avoid code duplication. Can you please do a small refactoring so that remove uses the new method, too? Further, the new method should not return a venv since it already takes the venv as argument. (It makes more sense that remove returns the venv because remove has to determine the venv itself.)

I was simply conservative and preferred not to change the tests. The TOML could be out of sync with the env folders for many reasons, for instance a user manually removing them from the cache because they take too much space on their device.

We can't expect the envs.toml to be consistent, but we should keep it consistent when running a Poetry command.

I you confirm that doing basic consistency checks makes sense, i'll considerer adding tests for that.

I think a test that checks that the entry is removed when calling remove --all makes sense. I even think we need two tests:

• the entry references an existing venv (removal can be handled in remove_venv)
• the entry references a non-existing venv (removal has to be handled outside of remove_venv because remove_venv removes exactly one venv and, thus, should only remove the entry if it matches the venv's python version)

[laclouis5]

I got some time to advance on this issue.

first, I don't think it would be straightforward to make remove to use remove_venv. There is a ton of logic in the remove (there re many nested if-statements and a code path even spin a subprocess) and I don't feel confident trying to stuff remove_env in here.

Second, I tried to implement the tests for remove --all but I stumbled on another issue: the current test suite does not seem to check anything related to the envs.toml, at least in the env removal tests. The env.toml file is literally not present from the temporary env folder created to test Poetry (with the sample simple-project).

I start to feel very confused about the purpose of that file and what is its expected state but I wasn't able to find any specification about it. Where can I find the documentation about Poetry's internals?

[radoering]

The current test suite does not seem to check anything related to the envs.toml, at least in the env removal tests.

In some places, tests are more incomplete than in others. There is a permanent issue to improve the tests suite.

The env.toml file is literally not present from the temporary env folder created to test Poetry (with the sample simple-project).

The file is probably not present because we do not always create a real virtualenv if we do not need one for a unit test. If we need an envs.toml file for a unit test we can just create it.

I start to feel very confused about the purpose of that file and what is its expected state but I wasn't able to find any specification about it. Where can I find the documentation about Poetry's internals?

Afaik, there is not such documentation. You have to look into the code.

I have just added some tests, improved the (in-code) documentation a bit and tried another approach, which I think covers more cases: #8831

[github-actions]

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.