fix: clean up dangling references when removing empty dependency groups#10653
Conversation
Reviewer's GuideThis PR ensures that when dependency groups become empty and are removed, any references to those groups in other groups’ include directives (both PEP 735 and legacy formats) are also cleaned up, and adjusts tests to match the new behavior and avoid key errors on partially-removed groups. Class diagram for updated remove command group cleanup logicclassDiagram
class RemoveCommand {
+handle() int
-_remove_packages(packages, standard_section, poetry_section, group_name) set
-_remove_references_to_group(group_name str, content dict_str_Any) void
}
class TomlContent {
+tool_poetry_group map
+dependency_groups map
}
RemoveCommand --> TomlContent : modifies
Flow diagram for _remove_references_to_group cleanup behaviorflowchart TD
A[Start _remove_references_to_group] --> B[Input group_name, content]
B --> C{content has dependency-groups?}
C -->|No| F
C -->|Yes| D[Iterate each group_content in content.dependency-groups.values]
D --> E{group_content is list?}
E -->|No| D
E -->|Yes| G[Collect items where item is dict and item.include-group == group_name]
G --> H[Remove collected items from group_content]
H --> D
D --> F[Get poetry_content = content.tool.poetry]
F --> I{poetry_content has group?}
I -->|No| Z[End]
I -->|Yes| J[Iterate each group_content in poetry_content.group.values]
J --> K{group_content has include-groups?}
K -->|No| J
K -->|Yes| L{group_name in group_content.include-groups?}
L -->|No| P{include-groups empty?}
L -->|Yes| M[Remove group_name from include-groups]
M --> P{include-groups empty?}
P -->|Yes| Q[Delete include-groups key]
P -->|No| J
Q --> J
J --> Z[End]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
_remove_references_to_group, the PEP 735 branch leaves empty lists independency-groupsafter removing allinclude-groupentries, whereas the legacy branch deletes emptyinclude-groups; consider normalizing behavior by also cleaning up now-empty lists or empty group entries for the PEP 735 path. - When removing legacy group dependencies in
handle, you now special-case deletion of thedependencieskey and then the group; it might be clearer and less error-prone to pull this into a small helper or to centralize the “group became empty” logic so the removal path for PEP 735 and legacy groups is handled consistently.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_remove_references_to_group`, the PEP 735 branch leaves empty lists in `dependency-groups` after removing all `include-group` entries, whereas the legacy branch deletes empty `include-groups`; consider normalizing behavior by also cleaning up now-empty lists or empty group entries for the PEP 735 path.
- When removing legacy group dependencies in `handle`, you now special-case deletion of the `dependencies` key and then the group; it might be clearer and less error-prone to pull this into a small helper or to centralize the “group became empty” logic so the removal path for PEP 735 and legacy groups is handled consistently.
## Individual Comments
### Comment 1
<location> `tests/console/commands/test_remove.py:445-448` </location>
<code_context>
assert "bar" not in pyproject.get("dependency-groups", {})
assert "dependency-groups" not in pyproject
else:
- assert "foo" not in content["group"]["bar"]["dependencies"]
- assert "baz" not in content["group"]["bar"]["dependencies"]
+ if "group" in content and "bar" in content["group"]:
+ assert "foo" not in content["group"]["bar"]["dependencies"]
+
+ assert "baz" not in content.get("group", {}).get("bar", {}).get("dependencies", {})
content = cast("TOMLDocument", content)
assert "[tool.poetry.group.bar]" not in content.as_string()
</code_context>
<issue_to_address>
**issue (testing):** The new conditional weakens the assertion and may hide regressions in how groups are cleaned up.
With the new guards, we no longer assert anything if `group.bar` is removed: the `foo` assertion is skipped, and the `baz` assertion runs against an empty dict. The test no longer states whether we expect `bar` to be removed entirely or to remain without those dependencies.
To keep the expectation explicit, consider asserting directly on the final structure, for example:
- Assert `"bar" not in content.get("group", {})` when the group should be removed, or
- Assert `"bar" in content["group"]` and that its `"dependencies"` key is missing/empty when it should remain.
This way the test will fail if group cleanup behavior changes, instead of silently passing.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
radoering
left a comment
There was a problem hiding this comment.
Can you add tests for the new code please?
Further, if sourcery is right (I did not check), this should probably be addressed:
- In
_remove_references_to_group, the PEP 735 branch leaves empty lists independency-groupsafter removing allinclude-groupentries, whereas the legacy branch deletes emptyinclude-groups; consider normalizing behavior by also cleaning up now-empty lists or empty group entries for the PEP 735 path.
Address PR python-poetry#10653 feedback: - Clean up PEP 735 dependency-groups that become empty after their include-group references are removed (normalizes with legacy behavior) - Add test for include-group reference cleanup when groups are removed - Strengthen test assertions for empty group removal 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Feedback AddressedI've pushed a new commit that addresses the review feedback: 1. PEP 735 empty list cleanup (Sourcery + @radoering)The Key change: Added logic to track and remove groups that become empty due to include-group cleanup, while ensuring we don't prematurely delete the target group itself (which is handled by the caller). 2. Added test for reference cleanup (@radoering)Added
3. Strengthened test assertions (Sourcery)Updated All 23 tests in |
Address PR python-poetry#10653 feedback: - Clean up PEP 735 dependency-groups that become empty after their include-group references are removed (normalizes with legacy behavior) - Add test for include-group reference cleanup when groups are removed - Strengthen test assertions for empty group removal 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
6226b86 to
b849db6
Compare
|
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. |
Fixes #10590.
When a dependency group becomes empty after removing packages, it is removed from the configuration. However, references to this group in other groups'
include-groups(Legacy) orinclude-group(PEP 735) were left dangling, causing errors.This PR adds logic to clean up these references when a group is removed.
Summary by Sourcery
Clean up dependency group references when groups become empty after package removal to prevent dangling references in the configuration.
Bug Fixes:
Tests: