Skip to content

Allow Ranges::contains to accept borrows, e.g. &str for Ranges<String>#301

Merged
Eh2406 merged 1 commit intodevfrom
konsti/dev/borrow
Dec 20, 2024
Merged

Allow Ranges::contains to accept borrows, e.g. &str for Ranges<String>#301
Eh2406 merged 1 commit intodevfrom
konsti/dev/borrow

Conversation

@konstin
Copy link
Member

@konstin konstin commented Dec 20, 2024

This PR upstreams astral-sh#35.


This PR borrows a trick from
HashMap to enable users to pass (e.g.) &str to Ranges::contains, given Ranges<String>.

…#35)

## Summary

This PR borrows a trick from
[HashMap](https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.contains_key)
to enable users to pass (e.g.) `&str` to `Ranges::contains`, given
`Ranges<String>`.
@Eh2406 Eh2406 added this pull request to the merge queue Dec 20, 2024
Merged via the queue into dev with commit 7767ef2 Dec 20, 2024
@Eh2406 Eh2406 deleted the konsti/dev/borrow branch December 20, 2024 16:00
@Eh2406
Copy link
Member

Eh2406 commented Dec 20, 2024

What is our process for releasing a new version of this crate?

@konstin
Copy link
Member Author

konstin commented Dec 20, 2024

It's updating Changelog.md, bumping the version number and running cargo publish locally.

@Eh2406
Copy link
Member

Eh2406 commented Dec 24, 2024

Two thoughts about this PR.

  1. We should probably add a test, even if it's just to compile test, for some basic use cases unlocked by this PR.
  2. Do we want hash maps API here? Seems like what we really need is Q: PartialOrd<V> not V: Borrow<Q>?

@Eh2406
Copy link
Member

Eh2406 commented Dec 26, 2024

Do we want hash maps API here? Seems like what we really need is Q: PartialOrd<V> not V: Borrow<Q>?

Q: PartialOrd<V> gets everything compile, and looks quite nice. Unfortunately it doesn't work for

can't compare `str` with `std::string::String`
the trait `PartialOrd<std::string::String>` is not implemented for `str`
the trait `PartialOrd` is implemented for `str`
for that trait implementation, expected `str`, found `std::string::String`

So moving on with my life.

Eh2406 added a commit to Eh2406/pubgrub that referenced this pull request Dec 26, 2024
@konstin
Copy link
Member Author

konstin commented Jan 2, 2025

This works, but i don't know if it's better:

use std::ops::Deref;

struct Db<V>(Vec<V>);

impl<V> Db<V> {
    pub fn contains<'a, Q, CommonCmp>(&self, version: &'a Q) -> bool
    where
        // We deref both internal and external type to this common type that we use for comparison.
        CommonCmp: PartialOrd<CommonCmp> + ?Sized,
        V: Deref<Target = CommonCmp>,
        &'a Q: Deref<Target = CommonCmp>,
        Q: PartialOrd<CommonCmp> + ?Sized,
    {
        self.0
            .binary_search_by(|segment| segment.deref().partial_cmp(version.deref()).unwrap())
            // An equal interval is one that contains the version
            .is_ok()
    }
}

fn main() {
    let haystack = Db(vec!["1".to_string(), "2".to_string(), "3".to_string()]);
    let one = haystack.contains("1");
    println!("1: {}", one);
    let four = haystack.contains(&"4".to_string());
    println!("4: {}", four);
}

github-merge-queue bot pushed a commit that referenced this pull request Jan 2, 2025
konstin added a commit to astral-sh/pubgrub that referenced this pull request Nov 13, 2025
github-merge-queue bot pushed a commit that referenced this pull request Nov 13, 2025
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