Combine dependency clauses with the same root #3225
Conversation
| @@ -366,7 +366,7 @@ fn excluded_only_compatible_version() { | |||
| And because you require one of: | |||
| package-a<2.0.0 | |||
| package-a>2.0.0 | |||
| and you require package-b>=2.0.0,<3.0.0, we can conclude that the requirements are unsatisfiable. | |||
| and package-b>=2.0.0,<3.0.0, we can conclude that the requirements are unsatisfiable. | |||
There was a problem hiding this comment.
This one I'm a little unsure about, but I think it's still clearer.
There was a problem hiding this comment.
Hm interesting thanks for flagging. It does seem like an improvement, I think.
There was a problem hiding this comment.
Just noticed the extra whitespace.
There was a problem hiding this comment.
I wonder if you require package-a but also package-b would be better in general.
There was a problem hiding this comment.
I'm not sure that generalizes well for the "proof" that's being stated.
|
@zanieb you mentioned that we might want to combine clauses within PubGrub itself. Is there a chance for false reporting with a transformation like this during formatting or does modifying PubGrub just allow for more types of simplification? |
|
Are you referring to #1673 (comment) or something else? This looks like a really reasonable step. In contrast to #1673, this implementation seems quite safe. I don't think this could false-report. I believe the alternative to handling this in the formatter is to derive a new tree with some collapsed nodes. We could look into implementing this in the default PubGrub formatter as well to improve the error messages it displays. I'm not sure we could collapse the nodes in PubGrub's resolver — that's more like #1901 which requires pubgrub-rs/pubgrub#208. We may also be able to use this pattern this for other incompatibility types e.g. instead of "because foo is unavailable and bar is unavailable" we could say "because foo and bar are unavailable". |
|
Nice work :) |
ibraheemdev
force-pushed
the
combine-clauses
branch
from
e44f998 to
59e4be9
Compare
Includes astral-sh/packse#179 for #3225 ``` uv pip compile scripts/scenarios/requirements.in -o scripts/scenarios/requirements.txt --refresh-package packse --upgrade cargo dev fetch-python source .env ./scripts/sync_scenarios.sh ```
ibraheemdev
force-pushed
the
combine-clauses
branch
from
59e4be9 to
430ec37
Compare
|
Thank you so much for the progress here. Even this targeted fix is probably worth up streaming in pubgrub at some point. |
Summary
Simplifies dependency errors of the form
you require package-a and you require package-btoyou require package-a and package-b. Resolves #1009.