fix(uv): preserve extra/group names in conflict error messages#4
fix(uv): preserve extra/group names in conflict error messages#4beast-ofcourse wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesProxy Detection Update
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/uv-resolver/src/error.rs (1)
483-497: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment is now stale.
Line 484 says this collapses incompatibilities wrapping an
PubGrubPackageInner::Extrapackage, but the predicate now only targetsMarker(per the newis_proxy_packagehelper). Worth updating the comment to avoid confusing future readers, sinceExtra/Groupare now intentionally excluded.📝 Suggested doc fix
/// Given a [`DerivationTree`], collapse any [`External::FromDependencyOf`] incompatibilities - /// wrap an [`PubGrubPackageInner::Extra`] package. + /// wrap an internal [`PubGrubPackageInner::Marker`] proxy package. `Extra` and `Group` + /// proxies are intentionally preserved since they carry user-meaningful context. pub(crate) fn collapse_proxies(derivation_tree: ErrorTree) -> ErrorTree {The rest of the logic change (narrowing to
Markeronly) is well-supported by the enum documentation incrates/uv-resolver/src/pubgrub/package.rsand correctly aligns with the PR's stated intent of preservingExtra/Groupcontext in error trees.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/uv-resolver/src/error.rs` around lines 483 - 497, The doc comment for collapse_proxies is stale and still mentions collapsing External::FromDependencyOf incompatibilities wrapping PubGrubPackageInner::Extra, but the new is_proxy helper only matches PubGrubPackageInner::Marker. Update the comment to describe that this function collapses marker-only proxy dependencies and intentionally preserves Extra and Group context, so the documentation matches the current behavior of collapse_proxies and is_proxy_package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/uv-resolver/src/error.rs`:
- Around line 483-497: The doc comment for collapse_proxies is stale and still
mentions collapsing External::FromDependencyOf incompatibilities wrapping
PubGrubPackageInner::Extra, but the new is_proxy helper only matches
PubGrubPackageInner::Marker. Update the comment to describe that this function
collapses marker-only proxy dependencies and intentionally preserves Extra and
Group context, so the documentation matches the current behavior of
collapse_proxies and is_proxy_package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c6279488-d8b6-418e-b0d2-c87cdfa15f47
📒 Files selected for processing (1)
crates/uv-resolver/src/error.rs
550dcf3 to
22a4923
Compare
- marker: None -> marker: None.into() (MarkerTree, not Option<MarkerTree>) - pubgrub::Range::any() -> pubgrub::Range::full() (removed associated fn) - &**p -> &***p (extra deref through Arc<PubGrubPackageInner>)
The collapse_proxies function in error reporting was removing all proxy nodes (extra, group, and marker) from the derivation tree during error formatting. This caused conflict error messages to lose extra and group names, e.g. showing \A depends on B >= 1\ instead of \A[extra] depends on B >= 1.
This change restricts proxy collapsing to only Marker variants, since marker info is a resolution-internal detail. Extra and group variants carry user-meaningful information about which optional dependencies are involved in the conflict.
Fixes astral-sh#19525
Summary by CodeRabbit