fix poetry add with PEP 735 dependency-groups with include-group#10636
Merged
radoering merged 1 commit intopython-poetry:mainfrom Dec 7, 2025
Merged
fix poetry add with PEP 735 dependency-groups with include-group#10636radoering merged 1 commit intopython-poetry:mainfrom
poetry add with PEP 735 dependency-groups with include-group#10636radoering merged 1 commit intopython-poetry:mainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts how Class diagram for updated dependency tracking in poetry addclassDiagram
class AddCommand {
- project_dependency_names list~NormalizedName_or_placeholder~
+ handle() int
+ get_existing_packages_from_input(packages list~str~, section dict~str_Any~, project_dependencies Collection~NormalizedName_or_placeholder~) list~str~
}
class NormalizedName {
}
class NormalizedName_or_placeholder {
<<type_alias>>
+ NormalizedName
+ literal_placeholder <include-group>
}
class Dependency {
+ create_from_pep_508(spec str) Dependency
+ name NormalizedName
}
AddCommand --> Dependency : uses
AddCommand --> NormalizedName_or_placeholder : tracks
NormalizedName_or_placeholder --> NormalizedName : includes
File-Level Changes
Assessment against 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:
- Consider extracting the string literal "" into a module-level constant or type alias so that the sentinel is defined in one place and not repeated in both the type hints and logic.
- The union type
NormalizedName | Literal["<include-group>"]is now used in multiple places; introducing a dedicated type alias (e.g.,ProjectDependencyName) would improve readability and make future changes to that union easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the string literal "<include-group>" into a module-level constant or type alias so that the sentinel is defined in one place and not repeated in both the type hints and logic.
- The union type `NormalizedName | Literal["<include-group>"]` is now used in multiple places; introducing a dedicated type alias (e.g., `ProjectDependencyName`) would improve readability and make future changes to that union easier.
## Individual Comments
### Comment 1
<location> `tests/console/commands/test_add.py:648-651` </location>
<code_context>
"cachy (>=0.2.0,<0.3.0)",
+ {include-group = "included"},
]
+included = []
"""
)
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test where the included dependency group itself contains dependencies to ensure it isn’t affected by the `add` operation.
With `included = []`, we only check that having an `include-group` entry doesn’t break `poetry add`, but we don’t verify that the included group’s contents are preserved. To strengthen this regression test, populate `included` with a dependency (e.g. `"some-package (>=1.0.0,<2.0.0)"`) and assert that `included` is unchanged after `poetry add`, while `example` is updated. This better protects against regressions where the index-based replacement logic mutates the wrong group.
Suggested implementation:
```python
included = ["some-package (>=1.0.0,<2.0.0)"]
"""
```
```python
assert "example" in pyproject["dependency-groups"]
assert pyproject["dependency-groups"]["example"] == [
"cachy (>=0.2.0,<0.3.0)",
{"include-group": "included"},
"pendulum (>=1.4.4,<2.0.0)",
]
# Ensure that the included group's contents are preserved by `poetry add`
assert pyproject["dependency-groups"]["included"] == [
"some-package (>=1.0.0,<2.0.0)",
]
if additional_poetry_group:
```
</issue_to_address>
### Comment 2
<location> `tests/console/commands/test_add.py:646-651` </location>
<code_context>
[dependency-groups]
example = [
"cachy (>=0.2.0,<0.3.0)",
+ {include-group = "included"},
]
+included = []
</code_context>
<issue_to_address>
**suggestion (testing):** Add a dedicated test case with multiple `include-group` entries in the same dependency group to cover index-based replacement edge cases.
The updated tests only cover a single `{include-group = "included"}` entry at different positions. Because the implementation does index-based replacement, please also add a case where a group contains multiple `include-group` entries (e.g. `[{include-group = "included-a"}, {include-group = "included-b"}, "cachy ..."]`) and verify that `poetry add` updates the concrete dependency while preserving all `include-group` entries.
Suggested implementation:
```python
[dependency-groups]
example = [
{include-group = "included-a"},
"cachy (>=0.2.0,<0.3.0)",
{include-group = "included-b"},
]
included-a = []
included-b = []
"""
)
pyproject["dependency-groups"] = groups_content["dependency-groups"]
assert "example" in pyproject["dependency-groups"]
assert pyproject["dependency-groups"]["example"] == [
{"include-group": "included-a"},
"cachy (>=0.2.0,<0.3.0)",
{"include-group": "included-b"},
"pendulum (>=1.4.4,<2.0.0)",
]
if additional_poetry_group:
```
To fully align with your review comment ("Add a dedicated test case"), you may want to:
1. Extract this scenario into its own test function (e.g. `test_add_to_dependency_group_with_multiple_include_groups`) by copying the surrounding test setup/teardown code and using the modified dependency-groups content there.
2. Keep the original single-`include-group` test as-is (revert this change in that original test) and place the multi-`include-group` variant in the new test to ensure both single and multiple include-group scenarios are covered.
3. If the `poetry add` invocation in this test is meant to *update* an existing dependency rather than only add a new one, also assert that the updated constraint for the concrete dependency (e.g. `"cachy (...)"`) is correctly changed while both `{"include-group": "included-a"}` and `{"include-group": "included-b"}` remain present and in the same relative positions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
bf67798 to
8acc4e5
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. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Resolves: #10632
Summary by Sourcery
Fix handling of PEP 735 dependency groups in
poetry addwhen groups containinclude-groupentries, ensuring dependencies are correctly updated without errors.Bug Fixes:
poetry addcorrectly processes dependency groups that includeinclude-groupitems by tracking them alongside normal dependencies.Tests:
poetry addcommand tests to cover dependency groups containinginclude-groupentries and verify correct group contents after additions.