-
Notifications
You must be signed in to change notification settings - Fork 261
fix: ensure all poetry-dependencies are added to lock file (#9959) #810
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 Guide by SourceryThis pull request fixes a bug where dependencies defined in the Sequence diagram for dependency locking processsequenceDiagram
participant DG as DependencyGroup
participant Lock as LockProcess
DG->>DG: dependencies_for_locking()
Note over DG: Create poetry_dependencies_by_name dict
loop For each dependency
alt dependency name in poetry_dependencies_by_name
Note over DG: Check for enrichment with poetry deps
alt enrichment possible
DG->>DG: Enrich dependency
DG->>DG: Track enriched poetry dependency
else no enrichment
DG->>DG: Keep original dependency
end
else dependency not in poetry_dependencies
DG->>DG: Keep original dependency
end
end
loop For remaining poetry dependencies
Note over DG: Add non-enriched poetry dependencies
DG->>DG: Add to dependencies list
end
DG-->>Lock: Return complete dependencies list
Class diagram for DependencyGroup changesclassDiagram
class DependencyGroup {
-_dependencies: list[Dependency]
-_poetry_dependencies: list[Dependency]
+dependencies_for_locking() list[Dependency]
+is_optional() bool
}
note for DependencyGroup "Modified to ensure all poetry
dependencies are included
in final lock list"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 and they look great!
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.
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.
Originally, I thought you should not be able to add additional dependencies via tool.poetry
if you are using project.dependencies
; you should only be able to enrich project.dependencies
. However, silently ignoring them is not a good option either. I think my original thought should still hold true for building but maybe we can relax it for locking.
However, I think the underlying issue in python-poetry/poetry#9959 is deeper and not fixed by the PR: If you mix project.optional-dependencies
and dynamic project.dependencies
via tool.poetry.dependencies
, the dependencies will also be missing when building a wheel. The issue is that optional dependencies go into the same dependency group as normal dependencies. The logic inside dependency groups works only well if there is one section in project
and one section in tool.poetry
, not if there are two sections each and you you are only using one in project
and want to have the other dynamic.
I am not sure if fixing the underlying issue will make this PR redundant. Probably not. I will try to find a solution for the underlying issue tomorrow if nobody beats me to it and would like to keep this one open until it is clear if we need it or not.
Resolves: python-poetry/poetry#9959
Summary by Sourcery
Bug Fixes: