env: improve error handling if .venv is not a directory but a file#10777
Merged
radoering merged 1 commit intopython-poetry:mainfrom Mar 28, 2026
Merged
env: improve error handling if .venv is not a directory but a file#10777radoering merged 1 commit intopython-poetry:mainfrom
.venv is not a directory but a file#10777radoering merged 1 commit intopython-poetry:mainfrom
Conversation
Reviewer's GuideImproves EnvManager’s handling of in-project Sequence diagram for EnvManager.create_venv when .venv is a filesequenceDiagram
actor Developer
participant PoetryCLI
participant EnvManager
participant Path_venv
participant IO
participant VirtualEnv
Developer->>PoetryCLI: run_command
PoetryCLI->>EnvManager: create_venv(name, io, options)
EnvManager->>Path_venv: is_dir()
alt venv_is_not_directory
EnvManager->>Path_venv: is_file()
alt venv_is_file
EnvManager->>IO: write_error_line(<warning>.venv is not a virtual environment but a file. Removing it.)
EnvManager->>Path_venv: unlink()
else venv_path_missing_or_other
Note over EnvManager,Path_venv: No file removal is performed
end
EnvManager->>IO: write_error_line(Creating virtualenv ...)
EnvManager->>VirtualEnv: create_at(venv_path, name)
VirtualEnv-->>EnvManager: venv_instance
EnvManager-->>PoetryCLI: VirtualEnvEnv
else venv_is_directory
EnvManager->>VirtualEnv: use_existing(venv_path)
VirtualEnv-->>EnvManager: venv_instance
EnvManager-->>PoetryCLI: VirtualEnvEnv
end
PoetryCLI-->>Developer: command_result
Updated class diagram for EnvManager virtualenv handlingclassDiagram
class EnvManager {
- IO _io
- Path in_project_venv
+ use_in_project_venv() bool
+ activate(python) Env
+ create_venv(name, io, options) Env
+ get_system_env() Env
}
class VirtualEnv {
+ version_info tuple
+ VirtualEnv(path)
}
class IO {
+ write_error_line(message) void
}
class Path {
+ is_dir() bool
+ is_file() bool
+ exists() bool
+ unlink() void
}
class Env {
}
EnvManager --> IO : uses
EnvManager --> Path : manages in_project_venv
EnvManager --> VirtualEnv : creates_and_checks
VirtualEnv --> Env : returns
Path <|.. Path_venv : instance
%% Key behavioral changes
%% - activate now checks in_project_venv.is_dir() instead of exists()
%% - create_venv treats paths where !is_dir() as invalid
%% - create_venv checks is_file(), warns via IO, and calls unlink() before creating a new VirtualEnv
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- When unlinking the unexpected
.venvfile, consider wrappingvenv.unlink()in a try/except forOSErrorand surfacing a clearer error or fallback behavior in case of permission or race-condition failures. - The special-case handling only covers
.venvbeing a regular file; if.venvis a broken symlink or other non-directory path, you might want to treat it consistently (e.g., warn and remove) rather than relying solely onis_dir().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When unlinking the unexpected `.venv` file, consider wrapping `venv.unlink()` in a try/except for `OSError` and surfacing a clearer error or fallback behavior in case of permission or race-condition failures.
- The special-case handling only covers `.venv` being a regular file; if `.venv` is a broken symlink or other non-directory path, you might want to treat it consistently (e.g., warn and remove) rather than relying solely on `is_dir()`.
## Individual Comments
### Comment 1
<location path="tests/utils/env/test_env_manager.py" line_range="484" />
<code_context>
+ venv_flags_default: dict[str, bool],
+ mocked_python_register: MockedPythonRegister,
+) -> None:
+ if "VIRTUAL_ENV" in os.environ:
+ del os.environ["VIRTUAL_ENV"]
+
</code_context>
<issue_to_address>
**suggestion (testing):** Use pytest's `monkeypatch` fixture instead of manually mutating `os.environ`.
Directly changing `os.environ` in tests can leak state between tests if an exception occurs before cleanup. Instead, take a `monkeypatch` fixture and use `monkeypatch.delenv("VIRTUAL_ENV", raising=False)` (or `setenv`/`delenv` as appropriate) so the environment is automatically restored and tests remain isolated.
Suggested implementation:
```python
def test_activate_with_in_project_setting_if_venv_is_file(
manager: EnvManager,
poetry: Poetry,
io: BufferedIO,
config: Config,
tmp_path: Path,
mocker: MockerFixture,
venv_flags_default: dict[str, bool],
mocked_python_register: MockedPythonRegister,
monkeypatch,
) -> None:
```
```python
) -> None:
monkeypatch.delenv("VIRTUAL_ENV", raising=False)
from cleo.io.buffered_io import BufferedIO
from pytest import LogCaptureFixture
from pytest_mock import MockerFixture
```
</issue_to_address>
### Comment 2
<location path="tests/utils/env/test_env_manager.py" line_range="516-518" />
<code_context>
+ envs_file = TOMLFile(tmp_path / "virtualenvs" / "envs.toml")
+ assert not envs_file.exists()
+
+ assert (
+ f"{venv_path} is not a virtual environment but a file. Removing it."
+ in io.fetch_error()
+ )
+
</code_context>
<issue_to_address>
**suggestion (testing):** Also assert that the `.venv` file was actually removed to fully validate the new behavior.
Right now the test only checks the warning and that `build_venv` is called. Please also add an assertion like `assert not venv_path.exists()` after `manager.activate(...)` to confirm the `.venv` file is actually removed.
Suggested implementation:
```python
def test_activate_with_in_project_setting_if_venv_is_file(
manager: EnvManager,
poetry: Poetry,
```
```python
manager.activate(poetry, io)
# Ensure the .venv file that was masquerading as an environment is removed
assert not venv_path.exists()
```
Because I can only see part of the file, you may need to adjust the `SEARCH` pattern to match your actual code:
1. Ensure the second `SEARCH` block matches the exact `manager.activate(...)` line in `test_activate_with_in_project_setting_if_venv_is_file`. For example, it might be `manager.activate(poetry, io)` or `manager.activate(poetry, io, ...)`.
2. If the path to the file is not stored in `venv_path`, adapt the assertion accordingly, e.g.:
```python
venv_path = poetry.file.path.parent / ".venv"
...
assert not venv_path.exists()
```
3. If `venv_path` is defined earlier in the test with a different name, replace `venv_path` in the new assertion with that variable name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
|
Should we remove the file or simply error out with a better message? Feels wrong to remove something we did not create or manage. |
Member
Author
|
If there is a |
Member
|
Me neither; I think removing it and recreating it is the opinionated approach. Just feels somehow weird. I guess we can go with it and see what happens. It is unlikely that this is going to make disruptions, unless well |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Check List
Currently, if an in-project
.venvis a file instead of a directory, Poetry errors out withWith the fix, a warning is printed, the file is removed and a venv is created.
Summary by Sourcery
Handle in-project virtual environment path correctly when it is a file instead of a directory and ensure a proper virtualenv is created with user-visible warnings.
Bug Fixes:
Enhancements:
Tests: