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

Implement PartialOrd and Ord for http::Uri #407

Open
Luro02 opened this issue Mar 29, 2020 · 5 comments · May be fixed by #455
Open

Implement PartialOrd and Ord for http::Uri #407

Luro02 opened this issue Mar 29, 2020 · 5 comments · May be fixed by #455

Comments

@Luro02
Copy link

Luro02 commented Mar 29, 2020

I thought about an implementation like this

impl PartialOrd for Uri {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}

impl Ord for Uri {
    fn cmp(&self, other: &Self) -> Ordering {
        if self.eq(other) {
            Ordering::Equal
        } else {
            (self.scheme(), self.authority(), self.path_and_query())
                .cmp(&(other.scheme(), other.authority(), other.path_and_query()))
        }
    }
}

the problem is the components only implement PartialOrd.

Another option would be:

impl PartialOrd for Uri {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}

impl Ord for Uri {
    fn cmp(&self, other: &Self) -> Ordering {
        if self.eq(other) {
            Ordering::Equal
        } else {
            self.to_string().cmp(other.to_string())
        }
    }
}
@Luro02 Luro02 changed the title Implement PartialOrd, Ord and Hash for http::Uri Implement PartialOrd and Ord for http::Uri Mar 29, 2020
@dekellum
Copy link
Contributor

Do you have a use case in mind for this? Could you share?

@Luro02
Copy link
Author

Luro02 commented Dec 12, 2020

Do you have a use case in mind for this? Could you share?

Deriving the PartialOrd and Ord traits on types that contain the http::Uri type?

@dekellum
Copy link
Contributor

dekellum commented Dec 12, 2020

Do those types of yours suggest any particular ordering for the Uri component? What I'm (just a contributor) looking for is if there is a particularly natural ordering. For example, compared to your first suggested Ord impl, I might have chosen:

(self.authority().host(), self.authority().port(), self.scheme(), self.path_and_query())

...in that order, as being more natural.

@Luro02
Copy link
Author

Luro02 commented Dec 12, 2020

As far as I remember, there was no particular reason, why I proposed the above implementation. It was meant as a suggestion, how this could be implemented.

I think it does not even matter that the ordering makes sense (for my use case), because I only wanted it, so I do not have to implement PartialEq, Eq, PartialOrd, Ord and Hash manually for the structs that use http::Uri internally. (All the trait implementations have to agree with each other).

I am not very familiar with this crate, so I do not know if your ordering is more natural, but my proposal would work with crates that use http::Uri conditionally and fallback to String. (The ordering for http::Uri would be equivalent to its String equivalent)

@dekellum
Copy link
Contributor

Authority currently does a case-insensitive host PartialOrd which is different then ordering the output of to_string(). That might suggest this ordering is more consistent than my original suggestion:

(self.authority(), self.scheme(), self.path_and_query())

As far as implementation, it might be better to avoid the conditional for Eq:

impl PartialOrd for Uri {
    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
        Some(self.cmp(other))
    }
}
impl Ord for Uri {
    fn cmp(&self, r: &Self) -> Ordering {
        (self.authority(), self.scheme(), self.path_and_query())
            .cmp(&(r.authority(), r.scheme(), r.path_and_query()))
    }
}

...unless I'm missing something about that?

Users can still new-type the Uri (or Authority) to achieve alternative PartialOrd/Ord.

@dekellum dekellum linked a pull request Dec 14, 2020 that will close this issue
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 a pull request may close this issue.

2 participants