feat: support nesting dependency groups #837

finswimmer · 2025-02-10T09:40:14Z

Include dependency groups into other dependency groups was requested earlier, e.g. python-poetry/poetry#7494. Also PEP 735 describes this mechanism.

Added tests for changed code.
Updated documentation for changed code -> docs: add docs about include-groups poetry#10166

Summary by Sourcery

New Features:

Allow including dependencies from other groups within a dependency group.

Summary by Sourcery

Add support for nested dependency groups.

New Features:

Allow dependency groups to include other groups.

Tests:

Add tests for nested dependency groups.

Summary by Sourcery

Add support for nested dependency groups.

New Features:

Allow dependency groups to include other groups.

Tests:

Add tests for nested dependency groups.

sourcery-ai · 2025-02-10T09:40:18Z

Reviewer's Guide by Sourcery

This PR introduces support for nested dependency groups. It enables a dependency group to include other groups, extends the validation logic to detect cyclic inclusions, and updates the dependency resolution to incorporate dependencies from included groups. The implementation is supported by new tests that cover various nesting scenarios, including direct, indirect, cyclic, and unknown group references.

Sequence diagram for nested dependency group resolution

sequenceDiagram
    participant C as Client
    participant DG as DependencyGroup
    participant D as Dependency

    C->>DG: Access dependencies property
    DG->>DG: Call _resolve_included_dependency_groups()
    loop For each included DependencyGroup
        DG->>DG: Iterate over included_dependency_groups
        loop For each dependency in included group
            DG->>D: dependency.clone()
            D-->>DG: Returns cloned dependency
            DG->>D: dependency.move_to_groups([current_group.name])
        end
        DG->>DG: Append resolved dependency
    end
    DG-->>C: Return combined dependencies (local + included)

Class diagram for DependencyGroup and Dependency

classDiagram
    class DependencyGroup {
      - list~Dependency~ _dependencies
      - list~Dependency~ _poetry_dependencies
      - dict~NormalizedName, DependencyGroup~ _included_dependency_groups
      + dependencies: list~Dependency~
      + dependencies_for_locking: list~Dependency~
      + include_dependency_group(dependency_group: DependencyGroup): void
      + _resolve_included_dependency_groups(): list~Dependency~
    }

    class Dependency {
      + clone(): Dependency
      + move_to_groups(groups: Iterable~string~): void
    }

    %% Relationship: DependencyGroup uses Dependency
    DependencyGroup o-- Dependency : includes

File-Level Changes

Change	Details	Files
Introduce nested dependency groups support	Enabled including one dependency group into another Added validation to prevent self-inclusion and cyclic dependencies Updated dependency resolution to merge dependencies from included groups	`src/poetry/core/factory.py` `src/poetry/core/packages/dependency_group.py`
Enhanced cyclic dependency detection and error handling	Implemented cycle detection in group includes in the factory validation method Raised appropriate errors for directly or indirectly cyclic group inclusions	`src/poetry/core/factory.py`
Update dependency merging with included groups	Modified dependency getters to include dependencies from nested groups Implemented a helper method to resolve and clone dependencies from included groups Ensured correct reassignment of group names in merged dependencies	`src/poetry/core/packages/dependency_group.py` `src/poetry/core/packages/dependency.py`
Extend test coverage for nested dependency groups	Added tests for valid nested dependency groups and merged dependencies Created tests for self-referenced and cyclic dependency groups Included tests to verify error handling for undefined and duplicate group includes	`tests/test_factory.py` `tests/packages/test_dependency_group.py`
Update schema documentation	Modified the JSON schema for Poetry to reflect changes in dependency groups	`src/poetry/core/json/schemas/poetry-schema.json`

Tips and commands

Interacting with Sourcery

Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment
@sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment
@sourcery-ai review to trigger a new review!
Generate a plan of action for an issue: Comment @sourcery-ai plan on
an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

Enable or disable review features such as the Sourcery-generated pull request
summary, the reviewer's guide, and others.
Change the review language.
Add, remove or edit custom review instructions.
Adjust other review settings.

Getting Help

Contact our support team for questions or feedback.
Visit our documentation for detailed guides and information.
Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

finswimmer · 2025-02-10T12:38:30Z

@sourcery-ai review

sourcery-ai

Hey @finswimmer - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider adding a method to DependencyGroup to check if a group is already included to avoid the try-except block in _configure_package_dependencies.
The move_to_groups method in Dependency could use a more descriptive name.

Here's what I looked at during the review

🟡 General issues: 1 issue found
🟢 Security: all looks good
🟡 Testing: 1 issue found
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

src/poetry/core/factory.py

tests/packages/test_dependency_group.py

finswimmer · 2025-02-10T18:00:25Z

@sourcery-ai review

sourcery-ai

Hey @finswimmer - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider adding a method to DependencyGroup to check if a group is already included to avoid the try/except in Factory._configure_package_dependencies.
The move_to_groups method in Dependency could use a clearer name, like add_group_inheritance.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟡 Testing: 3 issues found
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

sourcery-ai · 2025-02-10T18:01:40Z

tests/packages/test_dependency_group.py

+
+    group.include_dependency_group(group_2)
+
+    assert [dep.name for dep in group.dependencies] == ["foo", "bar"]


suggestion (testing): Test edge case where included group is empty.

Add a test case where group_2 is empty to ensure correct behavior when including an empty group.

Suggested implementation:

# Existing tests above...

def test_include_empty_dependency_group() -> None: group_main = DependencyGroup(name="group") group_main.add_poetry_dependency( Dependency(name="foo", constraint="*", groups=["group"]) ) empty_group = DependencyGroup(name="empty") # Include empty dependency group group_main.include_dependency_group(empty_group) # Assert that including an empty group does not affect group_main's dependencies. assert [dep.name for dep in group_main.dependencies] == ["foo"] assert [dep.name for dep in group_main.dependencies_for_locking] == ["foo"]

tests/test_factory.py

sourcery-ai

We have skipped reviewing this pull request. It looks like we've already tried to review the commit 445a089 in this pull request and failed.

src/poetry/core/factory.py

tests/test_factory.py

finswimmer · 2025-02-13T05:17:56Z

@sourcery-ai review

sourcery-ai

Hey @finswimmer - I've reviewed your changes - here's some feedback:

Overall Comments:

Consider adding a method to Package to encapsulate the logic for adding dependencies from included groups, instead of handling it directly in _configure_package_dependencies.
The error message for cyclic dependencies could be improved to include the full cycle path for easier debugging.

Here's what I looked at during the review

🟢 General issues: all looks good
🟢 Security: all looks good
🟡 Testing: 1 issue found
🟢 Complexity: all looks good
🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

tests/test_factory.py

src/poetry/core/packages/dependency_group.py

src/poetry/core/packages/dependency.py

radoering · 2025-02-13T09:50:27Z

src/poetry/core/factory.py

                )

+            for group_name, group_config in tool_poetry["group"].items():


Is there a reason to iterate a second time over tool_poetry["group"].items() and not just adding included groups and dependencies in one iteration?

I can only include a dependency group into another one if I have an instance of this dependency group.

Makes sense. In that case, it is just unlucky that this is not covered by the tests. I suppose adding a variant of test_create_poetry_with_nested_dependency_groups with swapped dependency groups will cover this case.

src/poetry/core/factory.py

radoering · 2025-02-13T10:07:28Z

src/poetry/core/packages/dependency_group.py


    @property
    def dependencies_for_locking(self) -> list[Dependency]:
+        included_group_dependencies = self._resolve_included_dependency_groups()


It might make no difference at the moment, but I think this is wrong because _resolve_included_dependency_groups uses dependencies instead of dependencies_for_locking. I think we need two versions of _resolve_included_dependency_groups (or pass a parameter that tells which dependencies to use).

Actually I guess I need more information about what's the difference between dependencies and dependencies_for_locking 😅

I guess you do not need to know the difference to consider it wrong: It feels clear to me that nesting groups should not violate the distinction between the two.

Nevertheless: dependencies are used for metadata (when building), dependencies_for_locking are used for locking. This distinction is probably not that relevant for groups because we only need dependencies_for_locking and do not use dependencies but that does not mean that we should not care about implementing it correctly.

tests/test_factory.py

radoering · 2025-02-13T10:19:00Z

tests/test_factory.py

@@ -1060,3 +1060,247 @@ def test_poetry_build_system_dependencies(
    poetry = Factory().create_poetry(temporary_directory)

    assert set(poetry.build_system_dependencies) == expected
+
+
+def test_create_poetry_with_nested_dependency_groups(temporary_directory: Path) -> None:


parametrize with different names (test normalization).

tests/packages/test_dependency_group.py

tests/test_factory.py

src/poetry/core/packages/dependency.py

finswimmer force-pushed the nested-dependency-groups branch 2 times, most recently from acbfec1 to 2ee59c0 Compare February 10, 2025 12:34

sourcery-ai bot reviewed Feb 10, 2025

View reviewed changes

src/poetry/core/factory.py Show resolved Hide resolved

tests/packages/test_dependency_group.py Show resolved Hide resolved

finswimmer force-pushed the nested-dependency-groups branch from 5291fcc to 206465f Compare February 10, 2025 17:59

sourcery-ai bot reviewed Feb 10, 2025

View reviewed changes

finswimmer force-pushed the nested-dependency-groups branch from 206465f to 445a089 Compare February 10, 2025 18:14

finswimmer marked this pull request as ready for review February 10, 2025 18:19

sourcery-ai bot reviewed Feb 10, 2025

View reviewed changes

finswimmer requested a review from a team February 10, 2025 18:19

finswimmer force-pushed the nested-dependency-groups branch 2 times, most recently from 2f8aabd to f0c551f Compare February 10, 2025 18:50

finswimmer mentioned this pull request Feb 10, 2025

docs: add docs about include-groups python-poetry/poetry#10166

Draft

radoering requested changes Feb 12, 2025

View reviewed changes

src/poetry/core/factory.py Outdated Show resolved Hide resolved

tests/test_factory.py Outdated Show resolved Hide resolved

finswimmer force-pushed the nested-dependency-groups branch from f0c551f to cb50fd3 Compare February 13, 2025 05:17

finswimmer requested a review from radoering February 13, 2025 05:18

sourcery-ai bot reviewed Feb 13, 2025

View reviewed changes

tests/test_factory.py Show resolved Hide resolved

radoering requested changes Feb 13, 2025

View reviewed changes

finswimmer force-pushed the nested-dependency-groups branch 7 times, most recently from 15626f4 to 912bc7a Compare February 13, 2025 20:36

radoering reviewed Feb 14, 2025

View reviewed changes

tests/packages/test_dependency_group.py Outdated Show resolved Hide resolved

tests/test_factory.py Outdated Show resolved Hide resolved

src/poetry/core/packages/dependency.py Outdated Show resolved Hide resolved

finswimmer force-pushed the nested-dependency-groups branch from 912bc7a to 8272314 Compare February 14, 2025 12:37

finswimmer force-pushed the nested-dependency-groups branch 2 times, most recently from 3430202 to 61ca884 Compare February 14, 2025 14:37

finswimmer added 3 commits February 14, 2025 16:55

feat: include groups into groups

0537a71

feat: parse include-groups

5117d20

feat: Add support for dependency groups which include other groups only

c6ff173

finswimmer force-pushed the nested-dependency-groups branch from 61ca884 to c6ff173 Compare February 14, 2025 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support nesting dependency groups #837

feat: support nesting dependency groups #837

finswimmer commented Feb 10, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Feb 10, 2025 •

edited

Loading

Interacting with Sourcery

Customizing Your Experience

Getting Help

finswimmer commented Feb 10, 2025

sourcery-ai bot left a comment

finswimmer commented Feb 10, 2025

sourcery-ai bot left a comment

sourcery-ai bot Feb 10, 2025

sourcery-ai bot left a comment

finswimmer commented Feb 13, 2025

sourcery-ai bot left a comment

radoering Feb 13, 2025

finswimmer Feb 13, 2025

radoering Feb 13, 2025

radoering Feb 13, 2025

finswimmer Feb 13, 2025

radoering Feb 14, 2025

radoering Feb 13, 2025


		group.include_dependency_group(group_2)

		assert [dep.name for dep in group.dependencies] == ["foo", "bar"]

		)

		for group_name, group_config in tool_poetry["group"].items():

feat: support nesting dependency groups #837

Are you sure you want to change the base?

feat: support nesting dependency groups #837

Conversation

finswimmer commented Feb 10, 2025 • edited by sourcery-ai bot Loading

Summary by Sourcery

Summary by Sourcery

Summary by Sourcery

sourcery-ai bot commented Feb 10, 2025 • edited Loading

Reviewer's Guide by Sourcery

Sequence diagram for nested dependency group resolution

Class diagram for DependencyGroup and Dependency

File-Level Changes

Interacting with Sourcery

Customizing Your Experience

Getting Help

finswimmer commented Feb 10, 2025

sourcery-ai bot left a comment

Choose a reason for hiding this comment

finswimmer commented Feb 10, 2025

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Feb 10, 2025

Choose a reason for hiding this comment

sourcery-ai bot left a comment

Choose a reason for hiding this comment

finswimmer commented Feb 13, 2025

sourcery-ai bot left a comment

Choose a reason for hiding this comment

radoering Feb 13, 2025

Choose a reason for hiding this comment

finswimmer Feb 13, 2025

Choose a reason for hiding this comment

radoering Feb 13, 2025

Choose a reason for hiding this comment

radoering Feb 13, 2025

Choose a reason for hiding this comment

finswimmer Feb 13, 2025

Choose a reason for hiding this comment

radoering Feb 14, 2025

Choose a reason for hiding this comment

radoering Feb 13, 2025

Choose a reason for hiding this comment

finswimmer commented Feb 10, 2025 •

edited by sourcery-ai bot

Loading

sourcery-ai bot commented Feb 10, 2025 •

edited

Loading