-
Notifications
You must be signed in to change notification settings - Fork 259
feat: Support PEP 735 dependency groups #823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR implements full PEP 735 support by extending the Factory to parse and normalize a new [dependency-groups] section alongside legacy tool.poetry.group tables, propagating group metadata through PEP 508 and file/dir dependency creation, enforcing schema validation with duplicate and cycle detection, and adding comprehensive tests to cover both valid and error scenarios. ER diagram for dependency-groups and group includeserDiagram
DEPENDENCY_GROUPS {
string name PK
bool optional
}
DEPENDENCY {
string name PK
string version
string[] groups
}
DEPENDENCY_GROUPS ||--o{ DEPENDENCY : contains
DEPENDENCY_GROUPS ||--o{ DEPENDENCY_GROUPS : includes
DEPENDENCY }o--|| DEPENDENCY_GROUPS : belongs_to
Class diagram for updated Dependency creation and group propagationclassDiagram
class Dependency {
+with_groups(groups: Iterable[str]) Dependency
+create_from_pep_508(name: str, relative_to: Path|None = None, groups: Iterable[str]|None = None) Dependency
}
class FileDependency {
<<Dependency>>
+groups: Iterable[str]|None
}
class DirectoryDependency {
<<Dependency>>
+groups: Iterable[str]|None
}
class VCSDependency {
<<Dependency>>
+groups: Iterable[str]|None
}
class URLDependency {
<<Dependency>>
+groups: Iterable[str]|None
}
Dependency <|-- FileDependency
Dependency <|-- DirectoryDependency
Dependency <|-- VCSDependency
Dependency <|-- URLDependency
Dependency o-- "*" groups: Iterable[str]
FileDependency o-- "*" groups: Iterable[str]
DirectoryDependency o-- "*" groups: Iterable[str]
VCSDependency o-- "*" groups: Iterable[str]
URLDependency o-- "*" groups: Iterable[str]
Class diagram for Factory dependency group handlingclassDiagram
class Factory {
+_add_package_pep735_group_dependencies(package, group, dependencies) list[str]
+_add_package_poetry_group_dependencies(package, group, dependencies)
+_configure_package_dependencies(package, project, tool_poetry, dependency_groups, with_groups)
+_validate_dependency_groups(toml_data, result)
}
class ProjectPackage {
+add_dependency_group(group: DependencyGroup)
+has_dependency_group(name: str) bool
+dependency_group(name: str) DependencyGroup
}
class DependencyGroup {
+add_dependency(dep: Dependency)
+include_dependency_group(group: DependencyGroup)
+optional: bool
+name: str
}
Factory o-- ProjectPackage
ProjectPackage o-- "*" DependencyGroup
DependencyGroup o-- "*" Dependency
DependencyGroup o-- "*" DependencyGroup : includes
Flow diagram for dependency group normalization and cycle detectionflowchart TD
A[Read dependency-groups and tool.poetry.group sections] --> B[Normalize group names]
B --> C[Check for duplicate group names]
C --> D[Build group include graph]
D --> E[Detect cycles in group includes]
E --> F[Report errors if duplicates or cycles found]
F --> G[Proceed with group creation if valid]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
83710eb to
6473ff4
Compare
6473ff4 to
e442bb4
Compare
2d7ac12 to
0a50ac3
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Please add documentation for the new dependency groups feature - this is a significant change that needs to be well documented for users
- Link to the relevant issue number/tracking issue for PEP 735 implementation in the PR description
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c19b017 to
ed77a26
Compare
|
@sourcery-ai review |
|
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider consolidating the two loops in _configure_package_dependencies that handle dependency groups (one for dependency-groups and one for tool.poetry.group) to reduce repeated logic.
- If a dependency group exists in the pyproject data but lacks a corresponding tool.poetry.group entry, consider whether you want to apply default optional settings explicitly.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ed77a26 to
c34e8b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider consolidating the two loops in _configure_package_dependencies to reduce code duplication in processing dependency groups.
- Validate and normalize the dependency groups input early to ensure consistent types before iterating over them.
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
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c34e8b2 to
c501df5
Compare
radoering
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support for https://peps.python.org/pep-0735/#dependency-object-specifiers is missing, which might be confusing for users.
|
I'm sure there are many who are eagerly awaiting this, thank you. |
|
Has development migrated to #837? Or is this PR blocked on that one? I'm happy to do some work to help this come in. |
15a65e1 to
d023277
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
- The newly added dependency-groups-schema.json file appears empty; please populate it with the actual JSON schema so that validation of
dependency-groupsworks correctly. - _configure_package_dependencies is becoming quite large and does multiple passes over groups—consider extracting group initialization, constraint parsing, and include handling into smaller helper methods to improve readability and maintainability.
- _resolve_dependency_group_includes currently mutates the input dict in a loop for cycle detection; switching to a dedicated graph‐based algorithm or working on a copy could make the cycle detection logic clearer and side‐effect free.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The newly added dependency-groups-schema.json file appears empty; please populate it with the actual JSON schema so that validation of `dependency-groups` works correctly.
- _configure_package_dependencies is becoming quite large and does multiple passes over groups—consider extracting group initialization, constraint parsing, and include handling into smaller helper methods to improve readability and maintainability.
- _resolve_dependency_group_includes currently mutates the input dict in a loop for cycle detection; switching to a dedicated graph‐based algorithm or working on a copy could make the cycle detection logic clearer and side‐effect free.
## Individual Comments
### Comment 1
<location> `src/poetry/core/factory.py:453` </location>
<code_context>
+ ) -> dict[NormalizedName, list[NormalizedName]]:
+ resolved_groups: set[NormalizedName] = set()
+ included: dict[NormalizedName, list[NormalizedName]] = defaultdict(list)
+ while resolved_groups != set(dependency_groups):
+ for group, dependencies in dependency_groups.items():
+ if group in resolved_groups:
</code_context>
<issue_to_address>
Possible infinite loop in _resolve_dependency_group_includes if dependency_groups is malformed.
Consider adding a maximum iteration limit or an explicit error if the loop cannot make progress, to prevent potential infinite loops with malformed input.
</issue_to_address>
### Comment 2
<location> `src/poetry/core/factory.py:466` </location>
<code_context>
+ resolved_dependencies.append(dep)
+ else:
+ included_group = canonicalize_name(dep["include-group"])
+ if included_group in included[group]:
+ raise ValueError(
+ f"Cyclic dependency group include:"
</code_context>
<issue_to_address>
Cyclic dependency group detection may not catch all cycles.
The current logic only detects direct cycles. To handle indirect cycles, implement a more comprehensive cycle detection algorithm, such as depth-first search with a visited set.
</issue_to_address>
### Comment 3
<location> `src/poetry/core/factory.py:700` </location>
<code_context>
]
result["errors"] += tool_poetry_validation_errors
+ dependency_groups = toml_data.get("dependency-groups")
+ if dependency_groups is not None:
+ dependency_groups_validation_errors = [
</code_context>
<issue_to_address>
Validation error messages for dependency-groups may be unclear.
Replacing 'data' with 'dependency-groups' in error messages may not always yield clear results, especially for complex schema errors. Please review the formatting to ensure messages remain accurate and user-friendly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
dependency_groups_validation_errors = [
e.replace("data", "dependency-groups")
for e in validate_object(dependency_groups, "dependency-groups-schema")
]
=======
dependency_groups_validation_errors = [
(
f"dependency-groups: {e[5:]}" if e.startswith("data:") else
e.replace("data", "dependency-groups", 1) if e.startswith("data") else
e
)
for e in validate_object(dependency_groups, "dependency-groups-schema")
]
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/poetry/core/packages/dependency.py:338` </location>
<code_context>
+ @classmethod
+ def _normalize_dependency_group_names(
+ cls,
+ dependency_groups: dict[str, list[str | dict[str, str]]],
+ ) -> dict[NormalizedName, list[str | dict[str, str]]]:
</code_context>
<issue_to_address>
The groups parameter in create_from_pep_508 is not documented.
Please update the docstring to include the groups parameter for clarity.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @finswimmer - I've reviewed your changes - here's some feedback:
- The nested loops and branching in _configure_package_dependencies are getting quite large—consider splitting the PEP 735 vs legacy group resolution into smaller helper functions or classes to improve readability.
- _add_package_pep735_group_dependencies and _add_package_poetry_group_dependencies share similar logic—unifying or renaming them to better reflect their distinct responsibilities would reduce confusion.
- The duplicate‐name and include‐cycle checks in _validate_dependency_groups are intertwined and hard to follow; extracting them into separate validation helpers could simplify the overall logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The nested loops and branching in _configure_package_dependencies are getting quite large—consider splitting the PEP 735 vs legacy group resolution into smaller helper functions or classes to improve readability.
- _add_package_pep735_group_dependencies and _add_package_poetry_group_dependencies share similar logic—unifying or renaming them to better reflect their distinct responsibilities would reduce confusion.
- The duplicate‐name and include‐cycle checks in _validate_dependency_groups are intertwined and hard to follow; extracting them into separate validation helpers could simplify the overall logic.
## Individual Comments
### Comment 1
<location> `src/poetry/core/factory.py:98` </location>
<code_context>
+ dependencies: list[str | dict[str, str]],
+ ) -> list[str]:
+ group_includes = []
+ for constraint in dependencies:
+ if isinstance(constraint, str):
+ dep = Dependency.create_from_pep_508(
+ constraint,
+ relative_to=package.root_dir,
+ groups=[group.pretty_name],
+ )
+ group.add_dependency(dep)
+ else:
+ group_includes.append(constraint["include-group"])
+ return group_includes
+
</code_context>
<issue_to_address>
Potential KeyError if 'include-group' is missing in dict constraint.
Use constraint.get("include-group") and handle the missing key to prevent KeyError exceptions.
</issue_to_address>
### Comment 2
<location> `src/poetry/core/factory.py:397` </location>
<code_context>
+ # with no corresponding entry in dependency-groups
+ # and add dependency information for existing groups
+ poetry_include_groups = {}
+ for group_name, group_config in tool_poetry_groups.items():
+ poetry_include_groups[group_name] = group_config.get(
+ "include-groups", []
)
</code_context>
<issue_to_address>
Possible inconsistency in group name normalization between sources.
Ensure group names are normalized consistently across all sources to prevent mismatches due to case or separator differences.
Suggested implementation:
```python
poetry_include_groups = {}
+ def _normalize_group_name(name):
+ # Normalize group names: lowercase and replace underscores with dashes
+ return name.lower().replace("_", "-")
+
+ for group_name, group_config in tool_poetry_groups.items():
+ normalized_group_name = _normalize_group_name(group_name)
+ poetry_include_groups[normalized_group_name] = group_config.get(
+ "include-groups", []
+ )
```
If group names are used elsewhere in this function or file (e.g., when reading from or writing to `poetry_include_groups`), ensure those accesses also use the `_normalize_group_name()` function for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I refactored the logic for group includes a bit so that we use the same logic for PEP 735 groups and The PR should be ready for merging now but I think I will wait until I reviewed python-poetry/poetry#10130, in case I spot any issues while reviewing this one. |
…s as for tool.poetry.group) (python-poetry#823)
e765d93 to
085765f
Compare
…s as for tool.poetry.group) (#823)
Required-by: python-poetry/poetry#10130
Relates-to: python-poetry/poetry#9751
Summary by Sourcery
New Features:
Summary by Sourcery
Implement support for PEP 735 dependency groups in Poetry core, merging the new [dependency-groups] table with existing tool.poetry.group settings, propagating group metadata to dependencies, and adding comprehensive validation.
New Features:
Enhancements:
Tests: