-
Notifications
You must be signed in to change notification settings - Fork 256
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
feat: Support PEP 735 dependency groups #823
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request implements support for PEP 735 dependency groups by extending the package configuration and dependency creation logic. The changes update how pyproject.toml data is parsed to include a new dependency-groups section, propagate this configuration through the package setup, and adjust dependency instantiation to carry group information. Additionally, new tests and fixtures have been added to validate both proper and invalid configurations. Sequence Diagram for PEP 735 Dependency Groups ProcessingsequenceDiagram
participant P as pyproject.toml
participant F as Factory.configure_package
participant D as _configure_package_dependencies
participant G as DependencyGroup
participant Dep as Dependency.create_from_pep_508
P->>F: Read 'dependency-groups' and tool_poetry config
F->>D: Pass project, tool_poetry, and dependency_groups
D->>G: For each dependency group, create DependencyGroup
loop For each dependency constraint in group
D->>Dep: Create Dependency (with groups parameter)
Dep-->>D: Return Dependency instance
D->>G: Add Dependency to DependencyGroup
end
D->>F: Package dependencies configured
Updated Dependency Class Diagram with Group SupportclassDiagram
class Dependency {
- string name
- string constraint
- list extras
- list groups
+ Dependency(name, constraint, extras, groups)
+ with_constraint(constraint)
+ create_from_pep_508(name, relative_to, groups)
}
class VCSDependency {
- string url
+ VCSDependency(name, "git", url, extras, groups)
}
class URLDependency {
- string url
+ URLDependency(name, "http/https", url, extras, groups)
}
class FileDependency {
- string path
+ FileDependency(name, path, base, directory, extras, groups)
}
class DirectoryDependency {
- string path
+ DirectoryDependency(name, path, base, extras, groups)
}
Dependency <|-- VCSDependency
Dependency <|-- URLDependency
Dependency <|-- FileDependency
Dependency <|-- DirectoryDependency
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.
tests/test_factory.py
Outdated
@@ -844,6 +869,19 @@ def test_create_poetry_with_invalid_dev_dependencies(caplog: LogCaptureFixture) | |||
assert any("dev" in r.groups for r in poetry.package.all_requires) | |||
|
|||
|
|||
def test_create_poetry_with_invalid_dependency_groups() -> None: |
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.
suggestion (testing): Test more invalid group configurations.
The current test only covers one invalid case. Consider adding tests for other invalid scenarios, such as incorrect data types for dependency names, invalid characters in group names, or missing required fields.
Suggested implementation:
def test_create_poetry_with_invalid_dependency_groups() -> None:
with pytest.raises(RuntimeError) as e:
_ = Factory().create_poetry(
fixtures_dir / "project_with_invalid_dependency_groups", with_groups=False
)
expected = """\
The Poetry configuration is invalid:
- dependency-groups.testing[1] must be string
"""
assert str(e.value) == expected
def test_create_poetry_with_invalid_dependency_names() -> None:
with pytest.raises(RuntimeError) as e:
_ = Factory().create_poetry(
fixtures_dir / "project_with_invalid_dependency_names"
)
expected = """\
The Poetry configuration is invalid:
- dependencies.some_dependency must be string, got int
"""
assert str(e.value) == expected
def test_create_poetry_with_invalid_group_names() -> None:
with pytest.raises(RuntimeError) as e:
_ = Factory().create_poetry(
fixtures_dir / "project_with_invalid_group_names"
)
expected = """\
The Poetry configuration is invalid:
- dependency-groups.invalid-group-name! contains invalid characters
"""
assert str(e.value) == expected
def test_create_poetry_with_missing_required_fields() -> None:
with pytest.raises(RuntimeError) as e:
_ = Factory().create_poetry(
fixtures_dir / "project_with_missing_required_fields"
)
expected = """\
The Poetry configuration is invalid:
- dependency 'essential' is missing required field 'version'
"""
assert str(e.value) == expected
Make sure to create the corresponding fixture projects under the fixtures directory for each invalid configuration:
- project_with_invalid_dependency_names
- project_with_invalid_group_names
- project_with_missing_required_fields
These changes allow tests to cover multiple invalid scenarios as suggested.
c34e8b2
to
c501df5
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.
Support for https://peps.python.org/pep-0735/#dependency-object-specifiers is missing, which might be confusing for users.
Required-by: python-poetry/poetry#10130
Relates-to: python-poetry/poetry#9751
Summary by Sourcery
New Features:
Summary by Sourcery
New Features: