Skip to content
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

Override range to avoid patching pubgrub #3713

Closed
wants to merge 2 commits into from
Closed

Conversation

konstin
Copy link
Member

@konstin konstin commented May 21, 2024

Create a newtype struct PubGrubRange(pubgrub::range::Range<Version>) to avoid overriding the Display implementation on Range in pubgrub itself, reducing our diff with upstream. Used methods taking an &Self or returning Self were overridden, the rest of the methods succeeds through deref.

An additional advantage is that we can start using the range type in more places without depending on pubgrub directly (#4041).

The alternative would be moving range formatting into the error formatter, but this would make pubgrub's api strange, deviating from rust's Display standard.

konstin added 2 commits June 3, 2024 17:48
Create a newtype `struct PubGrubRange(pubgrub::range::Range<Version>)` to avoid overriding the `Display` implementation on `Range` in pubgrub itself, reducing our diff with upstream. Used methods taking an `&Self` or returning `Self` were overridden, the rest of the methods succeeds through deref.

The alternative would be moving range formatting into the error formatter, but this would make pubgrub's api strange, deviating from rust's `Display` standard.
@konstin konstin marked this pull request as ready for review June 3, 2024 15:50
@konstin konstin force-pushed the konsti/pubgrub-range branch from c627b8f to c256073 Compare June 3, 2024 15:51
@zanieb
Copy link
Member

zanieb commented Jun 6, 2024

We discussed this in Discord and we both think this is a lot of boilerplate and a bit of an ugly solution but I don't think there's a better way if you want the ergonomics of Display. The alternative is probably relying on a PubGrub formatter method that we call into explicitly? I am not sure resolving this divergence from upstream is necessarily worth the added code here. It seems like we'd be taking on more maintenance burden to keep our wrapper type up to date. However, @konstin mentioned wanting to use PubGrub ranges outside the resolver which may justify the effort?

@zanieb
Copy link
Member

zanieb commented Oct 8, 2024

@konstin can we close this?

cc @BurntSushi I feel like we were recently talking about using Range outside the resolver

@BurntSushi
Copy link
Member

I hadn't seen this PR before.

Personally, I think the added code here is fine and I think it's worthwhile to reduce our pubgrub diff as much as possible with upstream.

With that said, my understanding is that we want to use something like a pubgrub Range outside of uv-resolver in uv-pep508 for the MarkerTree implementation. I believe there was talk of making a new crate that both upstream pubgrub depends on and we depend on that exposes a Range type.

@zanieb zanieb closed this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants