Proposed Fix for Optional Dependency Removal Bug (#10703)#10718
Closed
jontyhuang wants to merge 1 commit intopython-poetry:mainfrom
Closed
Proposed Fix for Optional Dependency Removal Bug (#10703)#10718jontyhuang wants to merge 1 commit intopython-poetry:mainfrom
jontyhuang wants to merge 1 commit intopython-poetry:mainfrom
Conversation
Author
|
This is a simulated PR to test PR management workflows. No actual code changes are applied. |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a minimal test-like script that exercises Poetry’s handling of optional dependency removal via the public Project/Factory APIs, demonstrating the fix for issue #10703 (removing an optional dependency via CLI/Poetry API). 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:
- The added script runs logic at import time; consider wrapping the execution in an
if __name__ == "__main__":block so that importing this module (e.g., from tests or tools) does not have side effects. - Instead of adding a standalone
test_optional_dependency_removal.pyscript, consider turning this into a proper test within the existing test suite structure so it can be automatically executed and maintained with other tests. - The direct construction and use of
ProjectandFactory().create_poetry(project)may not match how projects are normally instantiated in the codebase; aligning this setup with existing helper functions or factory patterns used in other tests will make the repro more realistic and less brittle to internal API changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The added script runs logic at import time; consider wrapping the execution in an `if __name__ == "__main__":` block so that importing this module (e.g., from tests or tools) does not have side effects.
- Instead of adding a standalone `test_optional_dependency_removal.py` script, consider turning this into a proper test within the existing test suite structure so it can be automatically executed and maintained with other tests.
- The direct construction and use of `Project` and `Factory().create_poetry(project)` may not match how projects are normally instantiated in the codebase; aligning this setup with existing helper functions or factory patterns used in other tests will make the repro more realistic and less brittle to internal API changes.
## Individual Comments
### Comment 1
<location> `test_optional_dependency_removal.py:2` </location>
<code_context>
+# Test script to reproduce issue #10703 in poetry
+# Expected: poetry remove pylint should succeed
+from poetry.factory import Factory
+from poetry.project.project import Project
</code_context>
<issue_to_address>
**suggestion (testing):** The test should reflect the actual CLI behavior described in the PR (using `poetry remove`), not just the internal API.
The comment’s expectation is about the CLI (`poetry remove pylint`), but this script only exercises internal classes. To reliably demonstrate the CLI bug is fixed, consider adding an integration/end-to-end test that runs `poetry remove pylint` (or equivalent) via the existing CLI test harness in a temporary project and asserts on exit code and output, matching the behavior described in issue #10703.
Suggested implementation:
```python
import subprocess
import tempfile
from pathlib import Path
import pytest
@pytest.mark.integration
def test_optional_dependency_removal_via_cli_succeeds():
"""
Reproduces issue #10703 via the CLI.
The expectation from the original report is that running:
poetry remove pylint
in a project where pylint is declared as an optional dependency
should succeed (exit code 0) instead of erroring.
"""
with tempfile.TemporaryDirectory() as tmp_dir:
project_dir = Path(tmp_dir)
pyproject = project_dir / "pyproject.toml"
pyproject.write_text(
"""
[tool.poetry]
name = "test_project"
version = "0.1.0"
description = ""
authors = ["Test <test@example.com>"]
[tool.poetry.dependencies]
python = "^3.8"
pylint = { version = "*", optional = true }
[tool.poetry.extras]
lint = ["pylint"]
""".lstrip(),
encoding="utf-8",
)
# Run the CLI as a user would.
result = subprocess.run(
["poetry", "remove", "pylint"],
cwd=project_dir,
capture_output=True,
text=True,
)
# The primary signal that the bug is fixed is a successful exit code.
assert (
result.returncode == 0
), f"'poetry remove pylint' failed.\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}"
```
Depending on the existing test conventions in the Poetry codebase, you may want to:
1. Move this test into the existing CLI/integration test package (e.g. `tests/console/` or similar) and adjust the module name accordingly.
2. Replace the direct `subprocess.run` usage with Poetry's existing CLI test harness/runner (for example, a `CommandTester`, `CliRunner`, or repository-specific helper) if one is already used elsewhere for `poetry` CLI tests.
3. Align the marker (`@pytest.mark.integration`) with whatever marker or naming convention the project uses for end‑to‑end or CLI tests (e.g. `@pytest.mark.e2e`, `@pytest.mark.cli`, or none at all).
4. If the test suite assumes `poetry` is invoked via a specific entry point (like `python -m poetry` or a helper function), adjust the `subprocess.run` arguments to match that convention.
</issue_to_address>
### Comment 2
<location> `test_optional_dependency_removal.py:10-11` </location>
<code_context>
+project = Project('test_project')
+poetry = Factory().create_poetry(project)
+poetry.add_dependency('pylint', {'optional': True})
+poetry.remove_dependency('pylint')
+print("Optional dependency removed successfully.")
\ No newline at end of file
</code_context>
<issue_to_address>
**suggestion (testing):** Add assertions to verify the dependency was actually removed instead of relying on the absence of an exception and a print.
Right now this test passes as long as no exception is raised and a message is printed. To make it a meaningful automated test, add an assertion that `pylint` is actually removed (e.g., it no longer appears in the dependencies collection or in the relevant section of `pyproject.toml` / lockfile).
Suggested implementation:
```python
# Test script to reproduce issue #10703 in poetry
# Expected: poetry remove pylint should succeed
from poetry.factory import Factory
from poetry.project.project import Project
# Simulated project setup
project = Project('test_project')
poetry = Factory().create_poetry(project)
# Add optional dependency
poetry.add_dependency('pylint', {'optional': True})
# Sanity check that the dependency was added
assert 'pylint' in poetry.package.dependencies
# Remove the dependency
poetry.remove_dependency('pylint')
# Assert that the dependency was actually removed
assert 'pylint' not in poetry.package.dependencies
```
If the Poetry API in your codebase exposes dependencies differently (for example via `poetry.locker`, `poetry.pyproject`, or another attribute instead of `poetry.package.dependencies`), adjust the two `assert` lines to target the correct collection or pyproject/lockfile representation.
For example, you might need to change them to something like:
- `assert 'pylint' in poetry.pyproject.poetry_config['tool']['poetry']['dependencies']`
- `assert 'pylint' not in poetry.pyproject.poetry_config['tool']['poetry']['dependencies']`
depending on how `add_dependency` and `remove_dependency` are implemented in your version of Poetry.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -0,0 +1,11 @@ | |||
| # Test script to reproduce issue #10703 in poetry | |||
| # Expected: poetry remove pylint should succeed | |||
There was a problem hiding this comment.
suggestion (testing): The test should reflect the actual CLI behavior described in the PR (using poetry remove), not just the internal API.
The comment’s expectation is about the CLI (poetry remove pylint), but this script only exercises internal classes. To reliably demonstrate the CLI bug is fixed, consider adding an integration/end-to-end test that runs poetry remove pylint (or equivalent) via the existing CLI test harness in a temporary project and asserts on exit code and output, matching the behavior described in issue #10703.
Suggested implementation:
import subprocess
import tempfile
from pathlib import Path
import pytest
@pytest.mark.integration
def test_optional_dependency_removal_via_cli_succeeds():
"""
Reproduces issue #10703 via the CLI.
The expectation from the original report is that running:
poetry remove pylint
in a project where pylint is declared as an optional dependency
should succeed (exit code 0) instead of erroring.
"""
with tempfile.TemporaryDirectory() as tmp_dir:
project_dir = Path(tmp_dir)
pyproject = project_dir / "pyproject.toml"
pyproject.write_text(
"""
[tool.poetry]
name = "test_project"
version = "0.1.0"
description = ""
authors = ["Test <test@example.com>"]
[tool.poetry.dependencies]
python = "^3.8"
pylint = { version = "*", optional = true }
[tool.poetry.extras]
lint = ["pylint"]
""".lstrip(),
encoding="utf-8",
)
# Run the CLI as a user would.
result = subprocess.run(
["poetry", "remove", "pylint"],
cwd=project_dir,
capture_output=True,
text=True,
)
# The primary signal that the bug is fixed is a successful exit code.
assert (
result.returncode == 0
), f"'poetry remove pylint' failed.\nstdout:\n{result.stdout}\nstderr:\n{result.stderr}"Depending on the existing test conventions in the Poetry codebase, you may want to:
- Move this test into the existing CLI/integration test package (e.g.
tests/console/or similar) and adjust the module name accordingly. - Replace the direct
subprocess.runusage with Poetry's existing CLI test harness/runner (for example, aCommandTester,CliRunner, or repository-specific helper) if one is already used elsewhere forpoetryCLI tests. - Align the marker (
@pytest.mark.integration) with whatever marker or naming convention the project uses for end‑to‑end or CLI tests (e.g.@pytest.mark.e2e,@pytest.mark.cli, or none at all). - If the test suite assumes
poetryis invoked via a specific entry point (likepython -m poetryor a helper function), adjust thesubprocess.runarguments to match that convention.
Comment on lines
+10
to
+11
| poetry.remove_dependency('pylint') | ||
| print("Optional dependency removed successfully.") No newline at end of file |
There was a problem hiding this comment.
suggestion (testing): Add assertions to verify the dependency was actually removed instead of relying on the absence of an exception and a print.
Right now this test passes as long as no exception is raised and a message is printed. To make it a meaningful automated test, add an assertion that pylint is actually removed (e.g., it no longer appears in the dependencies collection or in the relevant section of pyproject.toml / lockfile).
Suggested implementation:
# Test script to reproduce issue #10703 in poetry
# Expected: poetry remove pylint should succeed
from poetry.factory import Factory
from poetry.project.project import Project
# Simulated project setup
project = Project('test_project')
poetry = Factory().create_poetry(project)
# Add optional dependency
poetry.add_dependency('pylint', {'optional': True})
# Sanity check that the dependency was added
assert 'pylint' in poetry.package.dependencies
# Remove the dependency
poetry.remove_dependency('pylint')
# Assert that the dependency was actually removed
assert 'pylint' not in poetry.package.dependenciesIf the Poetry API in your codebase exposes dependencies differently (for example via poetry.locker, poetry.pyproject, or another attribute instead of poetry.package.dependencies), adjust the two assert lines to target the correct collection or pyproject/lockfile representation.
For example, you might need to change them to something like:
assert 'pylint' in poetry.pyproject.poetry_config['tool']['poetry']['dependencies']assert 'pylint' not in poetry.pyproject.poetry_config['tool']['poetry']['dependencies']
depending on how add_dependency and remove_dependency are implemented in your version of Poetry.
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
1 participant
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.
This PR proposes a fix for issue #10703 where Poetry fails to remove optional dependencies via CLI. A test script is included to demonstrate the intended behavior.
Summary by Sourcery
Tests: