-
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
fix transitive_marker
handling
#10141
Conversation
Reviewer's Guide by SourceryThis pull request fixes an issue where the solver was not correctly handling transitive markers, leading to incorrect version resolution in certain scenarios. The changes include refactoring the version selection logic and adding a new test case to verify the fix. Sequence diagram for package version resolution with transitive markerssequenceDiagram
participant Solver as VersionSolver
participant Solution
participant Term
participant Dependency
Solver->>Solution: Get unsatisfied dependencies
Solution-->>Solver: Return list of dependencies
Solver->>Solver: _choose_next(unsatisfied)
Note over Solver: Select dependency based on preference
Solver->>Term: Check relation with other terms
Term->>Term: Compare transitive markers
Note over Term: New: Consider transitive markers<br/>when checking relations
Term->>Dependency: Union transitive markers
Note over Dependency: Merge markers when<br/>combining positive terms
Class diagram showing the modified version solver componentsclassDiagram
class VersionSolver {
-_solution: Solution
-_provider: Provider
+_choose_package_version(): str|None
+_choose_next(unsatisfied: list[Dependency]): Dependency
+_resolve_conflict(incompatibility: Incompatibility): Incompatibility
}
class Term {
+dependency: Dependency
+is_positive: bool
+_relation(other: Term): str
+_non_empty_term(other: Term, constraint: Any, is_positive: bool): Term
}
class Dependency {
+transitive_marker: Any
+complete_name: str
+with_constraint(constraint: Any): Dependency
}
VersionSolver ..> Term: uses
Term ..> Dependency: contains
note for Term "Modified to handle transitive markers"
note for Dependency "Enhanced with transitive marker handling"
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:
- Please add documentation explaining how transitive markers work and are handled in the dependency resolver. This will help users and contributors understand this important behavior.
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.
Without this fix, `test_solver_should_not_raise_errors_for_irrelevant_transitive_python_constraints` wrongly chooses the later `importlib-resources` version even though it is not compatible with the project's python constraint depending on which is resolved first, `virtualenv` or `pre-commit`, because the transitive marker of the other's dependency on `importlib-resources` is ignored.
b59d634
to
67ded88
Compare
Pull Request Check List
Without this fix,
test_solver_should_not_raise_errors_for_irrelevant_transitive_python_constraints
wrongly chooses the laterimportlib-resources
version even though it is not compatible with the project's python constraint depending on which is resolved first,virtualenv
orpre-commit
, because the transitive marker of the other's dependency onimportlib-resources
is ignored.The first commit is just refactoring to facilitate the test, added in the second commit.
Summary by Sourcery
Fix handling of transitive markers during dependency resolution. This addresses an issue where incompatible transitive dependencies could be selected due to incorrect marker handling.
Bug Fixes:
test_solver_should_not_raise_errors_for_irrelevant_transitive_python_constraints
would select an incompatibleimportlib-resources
version.Tests: