-
Notifications
You must be signed in to change notification settings - Fork 254
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
fix: do not ignore dynamic project dependencies via tool.poetry.dependencies
if project.optional-dependencies
are defined
#811
fix: do not ignore dynamic project dependencies via tool.poetry.dependencies
if project.optional-dependencies
are defined
#811
Conversation
Reviewer's Guide by SourceryThis pull request addresses a bug where dynamic project dependencies specified via Class diagram showing updated DependencyGroup classclassDiagram
class DependencyGroup {
-_dependencies: list[Dependency]
-_poetry_dependencies: list[Dependency]
+name: str
+dependencies(): list[Dependency]
+dependencies_for_locking(): list[Dependency]
}
note for DependencyGroup "Modified dependencies property to handle
optional and non-optional dependencies
from both sources correctly"
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 @radoering - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding documentation to explain the dependency merging behavior, though the comprehensive test suite does a good job of illustrating the expected behavior.
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.
@@ -26,7 +26,20 @@ def name(self) -> str: | |||
|
|||
@property | |||
def dependencies(self) -> list[Dependency]: | |||
return self._dependencies or self._poetry_dependencies | |||
if not self._dependencies: |
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.
I guess I understood what's going on here 💪 And it looks like it should work.
As far as I see, we don't take into account the dynamic
key of the project
section anywhere in our codebase. Instead we are doing educated guesses. Wouldn't it be easier to rely on this field?
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.
At first, I thought it is redundant. However, there is at least one use case where it is necessary:
Projects that do not have any mandatory dependencies but optional dependencies and use tool.poetry.dependencies
to define sources for these optional dependencies. Without taking project.dynamic
into account the dependencies in tool.poetry.dependencies
would have been considered mandatory dependencies in this case.
I added a second commit that takes project.dynamic
into account to fix this use case.
…ndencies` if `project.optional-dependencies` are defined
…cies are used to define or only to enrich project dependencies This is especially relevant for projects that do not have any mandatory dependencies but optional dependencies and use `tool.poetry.dependencies` to define sources for these optional dependencies. Without this change the dependencies in `tool.poetry.dependencies` would have been considered mandatory dependencies in this case.
3d67a46
to
ad3da61
Compare
Resolves: python-poetry/poetry#9959
Closes: #810
Summary:
project.dependencies
,project.optional-dependencies
andtool.poetry.dependencies
sample_project_dynamic
(everything dynamic that we did not deprecate) in addition to existing fixturessample_project
(legacy, noproject
section) andsample_project_new
(everything that is possible inproject
section)complete_dynamic
in addition to existing fixturescomplete
andcomplete_new
Summary by Sourcery
Include dynamic optional dependencies defined in the
tool.poetry.dependencies
section whenproject.optional-dependencies
are also defined.Bug Fixes:
tool.poetry.dependencies
were ignored ifproject.optional-dependencies
were defined.Tests: