-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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(cli): Support PEP 735 (Dependency Groups) #10130
base: main
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis PR implements support for PEP 735 by introducing the new 'dependency-groups' section to handle dependency groups in pyproject.toml. The changes span across CLI commands, tests, documentation, and internal package handling. The implementation adds conditional logic to support both the legacy [tool.poetry.group] structure and the new [dependency-groups] format, with tests parameterized on the PEP735 flag to ensure correct behavior in both modes. Sequence Diagram for Poetry Add Command with Dependency GroupssequenceDiagram
actor User
participant CLI_Add as "Poetry Add Command"
participant Config as "Pyproject.toml"
participant Locker as "Locker"
User->>CLI_Add: Run 'poetry add <package>' with group flag
CLI_Add->>Config: Read configuration
CLI_Add->>CLI_Add: Determine dependency group (MAIN group vs. dependency-groups)
CLI_Add->>Config: Update dependency entry in project section or dependency-groups
CLI_Add->>Locker: Update locker with new pyproject data
CLI_Add->>Config: Write back updated configuration
User->>CLI_Add: Command completes with success status
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
f468fca
to
eeb4814
Compare
@sourcery-ai review |
bacef77
to
ee49998
Compare
Deploy preview for website ready! ✅ Preview Built with commit e522f40. |
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.
Some feedback:
- To satisfy the export tests, you can try to add
poetry remove --lock poetry-core
inpoetry/.github/workflows/.tests-matrix.yaml
Line 115 in 9af386a
poetry add --lock --group dev ../poetry - The
support pep 735
commit is missing a test forremove
and the tests intest_remove_plugins
are stillxfail
. (If this is still on your todo list ignore the comment).
93e9c7e
to
628c412
Compare
720e60f
to
727e441
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:
- The remove command does not fully support the new dependency-groups format yet, as evidenced by the xfailed tests. This should be implemented before merging.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues 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.
727e441
to
1c9ed30
Compare
4642fc2
to
38f0b35
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:
- Extract the common dependency-group setup logic from tests into helper functions to reduce duplication in the if-else blocks.
- Refactor the repeated type casts and handling of groups_content to improve code readability and maintainability.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 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.
@@ -152,14 +152,18 @@ def handle(self) -> int: | |||
content: dict[str, Any] = self.poetry.file.read() | |||
project_content = content.get("project", table()) | |||
poetry_content = content.get("tool", {}).get("poetry", table()) | |||
groups_content = content.get("dependency-groups") |
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 (bug_risk): Consider defaulting groups_content to a dict.
While the removal command defaults groups_content to {} immediately, the add command obtains groups_content without a default value. Using a default (e.g. {} or an appropriate table) might avoid potential None-type issues and ensure consistency between commands.
groups_content = content.get("dependency-groups") | |
groups_content = content.get("dependency-groups", {}) |
@@ -27,14 +27,30 @@ are part of an implicit `main` group. Those dependencies are required by your pr | |||
Beside the `main` dependencies, you might have dependencies that are only needed to test your project |
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.
issue (typo): Typo: "Beside" should be "Besides".
Beside the `main` dependencies, you might have dependencies that are only needed to test your project | |
Besides the `main` dependencies, you might have dependencies that are only needed to test your project |
bf31cc9
to
db3ee35
Compare
db3ee35
to
674b002
Compare
674b002
to
e522f40
Compare
poetry-core
tomain
after feat: Support PEP 735 dependency groups poetry-core#823 is merged.Pull Request Check List
Requires: python-poetry/poetry-core#823
Resolves: #9751
Summary by Sourcery
Support PEP 735 by adding dependency groups to the pyproject.toml file.
New Features:
Tests:
Summary by Sourcery
Update dependency management to support PEP 735 (Dependency Groups).
New Features:
Tests: