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

Allow to access a Range's segments #258

Open
giacomocavalieri opened this issue Sep 23, 2024 · 17 comments
Open

Allow to access a Range's segments #258

giacomocavalieri opened this issue Sep 23, 2024 · 17 comments

Comments

@giacomocavalieri
Copy link

Hello! I ran into a problem while writing some custom error reporting for resolution failure for the Gleam compiler. What I do is I start from a DerivationTree, transform it a bit, and report a pretty printed error message showing the failing constraints along with version ranges. For example the following derivation tree:

// I'm omitting terms and ids and pretty printing the version ranges...
Derived {
  cause1: Derived {
    cause1:  FromDepOf {
      wisp: 1.1.0,
      gleam_json: 0.6.0 <= v < 2.0.0,
    },
    cause2:  FromDepOf {
      prova: 0.0.0,
      wisp: 1.1.0,
    },
  },
  cause2: FromDepOf {
    prova: 0.0.0,
    gleam_json: 2.0.0 <= v < 3.0.0,
  },
}

Gets turned into:

Unable to find compatible versions for the version constraints in your
gleam.toml:

- `prova` requires `wisp` to be `1.1.0`
- and all versions of `wisp` in that range require `gleam_json` to be
`0.6.0 <= v < 2.0.0`
- but `prova` also requires `gleam_json` to be `2.0.0 <= v < 3.0.0`
- and there is no version of `gleam_json` that satiesfies both constraints

One thing I'd like to change is the default look of version ranges once they're printed: for example having >= 2.0.0 and < 3.0.0 instead of 2.0.0 <= v < 3.0.0; this would look more familiar to a Gleam programmer since it more closely resembles how one would write a version constraint in Gleam.

My problem is that the only way I found to turn a Range<_> into a string is using the default fmt implementation and since the segments field is private I cannot roll my own pretty printing implementation for ranges. Could the package expose a way to access a range's segments?

Thank you!

@zanieb
Copy link
Member

zanieb commented Sep 23, 2024

Yeah this is a bit of a rough edge. Thank you for sharing your use-case! You may find the the uv report implementation interesting and helpful — we have customized the messages quite a bit.

I think you can access a range's segments with Range::iter(), e.g., for segment in range.iter()

@giacomocavalieri
Copy link
Author

You may find the the uv report implementation interesting and helpful — we have customized the messages quite a bit.

I'll have a look at it for sure, thank you!!

I think you can access a range's segments with Range::iter(), e.g., for segment in range.iter()

I'm not really that good of a Rustacean so I might be probably missing something, but it doesn't look like pubgrub::range::Range is implementing the iterator trait, if I try writing range.iter() I get:

No method named `iter` found for struct `pubgrub::range::Range` in the current scope
method not found in `Range<Version>`

@zanieb
Copy link
Member

zanieb commented Sep 23, 2024

I'll have a look at it for sure, thank you!!

Be warned it's quite complicated. Happy to help though.

... but it doesn't look like pubgrub::range::Range is implementing the iterator trait...

Hm it's implemented at

pubgrub/src/range.rs

Lines 835 to 838 in fe65959

/// Iterate over the parts of the range.
pub fn iter(&self) -> impl Iterator<Item = (&Bound<V>, &Bound<V>)> {
self.segments.iter().map(|(start, end)| (start, end))
}

@mpizenberg
Copy link
Member

One remark, it’s possible that gleam still depends on pubgrub 0.2.1 as no new version has been published since that. However, a lot has changed on the default dev branch. And uv is working from the latest code of the dev branch, not from v0.2.1. I don’t think that iterator was existing/exposed in version 0.2.1.

We do not intend to update version 0.2 of the pubgrub crate. It currently is stabilizing to a version 0.3 that would support uv features and others that @Eh2406 is working on for cargo. So my guess is you have three options @giacomocavalieri

  1. fork the v0.2.1 and add the function you need
  2. update to the current dev branch, which contains a lot of improvements, many related to error reporting. It has a slightly different api, but there should be an easy upgrade path, because in theory the new api is more expressive than the old one. So we can emulate the v0.2.1 api with the one from the dev branch
  3. wait for v0.3 to be published before updating. It can take a long time since a lot of documentation is needed.

@giacomocavalieri
Copy link
Author

@mpizenberg thank you for the really helpful answer!! Ideally I'd like to wait for v0.3, do you have a rough date in mind for when we could see a release or is it going to be really far into the future?

@Eh2406
Copy link
Member

Eh2406 commented Sep 24, 2024

So many deadlines have passed me by, I like the sound they make when they go past, it makes me hesitant to admit to anymore. But according to the document I share with my boss I'm supposed to have the next release out by the end of the year. Though, after committing to that I instead spent my time working on #232 which has been a lot trickier to understand/diagnose than I was expecting. 🤷

If you want to see progress before the official release I may be able to help by:

  • Back porting changes to make a v0.2.2 (I wasn't intending to do this, but for those changes that back porting easily I might consider it.)
  • Helping you with a PR to your project do the up graid to the dev branch.
  • If your project can't take GIT dependencies, cutting a v0.3.0-alpha with what we have. There will be some breaking changes before the official release. We often find naming improvements when finishing out the documentation.

@konstin
Copy link
Member

konstin commented Sep 24, 2024

I'd love to start doing alpha releases! Even if the docs aren't ready, this should get us into the process of eventually releasing 0.3.

@mpizenberg
Copy link
Member

a PR to your project do the up graid to the dev branch.

whoever does that, it will be a good learning point for improving the upgrade docs. Especially if that’s not Jacob doing the upgrade.

@giacomocavalieri
Copy link
Author

Thank you so much to everyone who replied!
@Eh2406 thank you for being so helpful, I really appreciate it! I don't want to distract you or take any of your time, I can gladly wait for a release by the end of the year or even later, I'm in no rush :)

@Alijocker369

This comment was marked as spam.

@konstin
Copy link
Member

konstin commented Nov 11, 2024

version-ranges is now published as its own crate (https://crates.io/crates/version-ranges), and we're also adding IntoIter support (#276)

@konstin konstin closed this as completed Nov 20, 2024
@giacomocavalieri
Copy link
Author

Hello! I'm not really sure how using the version-ranges package solves my issue, I have a Range<V> and see no way to turn that into a Ranges value I could then work with, sorry if I'm missing something dumb! 🙇

@konstin
Copy link
Member

konstin commented Nov 21, 2024

The released pubgrub 0.2 uses the Range types, while the dev branch uses version_ranges::Ranges. It has the same internal representation, but it's moved to its own crate and has gained some features and significant performance improvements since pubgrub 0.2. In that sense, the issue is "fixed on main", but we're lacking a new release (#192).

@lpil
Copy link
Contributor

lpil commented Nov 21, 2024

That's great news! Thank you everyone!

Do you have a rough idea of what it might be? So we can plan accordingly. Thanks again

@konstin
Copy link
Member

konstin commented Nov 26, 2024

Both uv and cargo currently used a pinned version based of the dev branch. I think doing an actual release it blocked on someone updating the book (#192 (comment)).

@lpil
Copy link
Contributor

lpil commented Nov 26, 2024

I see, thank you. Would you be open to the change being ported to the v0.2 branch?

Would it be possible to reopen this issue? We still cannot access the range segments. 🙏

@konstin konstin reopened this Nov 26, 2024
@Eh2406
Copy link
Member

Eh2406 commented Nov 27, 2024

Would you be open to the change being ported to the v0.2 branch?

Yes. We would need to be careful about breaking changes related to strictly_lower_than(5) == lower_than(4). The 0.2 implementation is limited to discrete values so knows them equal. The new impl can handle continuous values, and therefore thinks that there might be a version >4 and <5. I don't remember the 0.2 API well enough to know if we can just "add new constructors that don't require Bump" or if this will be a bigger problem.

As for 0.3 timeline. I'm still hoping to have a release out by the end of the year. Although, although every day it's more clearly going to be a pre-release.

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

No branches or pull requests

7 participants