Conversation
There was a problem hiding this comment.
Hey - I've reviewed your changes and they look great!
Sourcery assessment
Needs a human reviewer. If the matching logic is wrong, poetry remove could delete a dependency declaration from pyproject.toml, changing future environment resolution. Reverting the code would not restore declarations already removed, but the bounded configuration change can be repaired by restoring the entries.
86d9a04 to
da49e91
Compare
|
Explain why not #10734. See also #10726 (comment) |
Without `--group`, `remove` only looked at `[project.dependencies]` and
`[tool.poetry.dependencies]`. A dependency that `add --optional <extra>` wrote
into `[project.optional-dependencies]` was therefore never found, and removing
it failed with:
ValueError: The following packages were not found: foo
Search the `[project.optional-dependencies]` tables too, drop the extras that
become empty and remove the whole table when its last entry is gone.
Fixes python-poetry#10703.
da49e91 to
516e2f8
Compare
|
Thanks for looking at this, and for the pointer to #10726 — that comment is exactly the design question I hit while writing the patch. Both fix the same bug (#10703) in a different place than this PR does:
If you would rather have this inside #10734, say so and I will close this one and post the patch there — the behaviour matters more than whose PR carries it. On the semantics you raised in #10726 What this PR implements: You are right that two cases stay ambiguous, because
I did not invent syntax for them. The options as I see them:
I picked (a) because with no Two smaller notes:
|
Summary
poetry remove <package>never looked at[project.optional-dependencies], so a dependency added withpoetry add --optional <extra> <package>(a PEP 621 project) could not be removed:Without
--group,removecollected its candidates from[project.dependencies]and[tool.poetry.dependencies]only.add --optionalwrites the dependency into[project.optional-dependencies].<extra>when the project declares a[project]table, and that table was never searched, soremoveconcluded the package was not a dependency at all.Changes
src/poetry/console/commands/remove.py: search the[project.optional-dependencies]tables as well, match on the canonicalized PEP 508 name, drop the extras that become empty, and remove the wholeoptional-dependenciestable when its last entry is gone.collections.abc.Mappingis imported at runtime becauseisinstanceis now used in the module.tests/console/commands/test_remove.py: two regression tests — one removesfoofrom thelintingextra while leaving thetypecheckingextra untouched, the other removes the last entry and asserts the emptied table is gone.Validation
pytest tests/console/commands/test_remove.py -q -n0→ 27 passed.src/poetry/console/commands/remove.pymakes both new tests fail with the reportedValueError: The following packages were not found: foo, so the tests exercise the bug rather than the implementation.ruff checkandruff format --checkare clean on both files.Fixes #10703.
Disclosure: this change was prepared with an AI coding assistant (DeepSeek) under my direction; I reviewed the diff and the commands above before pushing.