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

Add impl From<Uri> for Location + accessor #104

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fasterthanlime
Copy link

As far as I know, all URI characters should be valid Header value
characters, so we can have an infallible constructor for Location
from an Uri.

This doesn't cover all uses of the Location header, since it allows
URI-references like /People.html#tim, but it's an ergonomic win
already, as mentioned in #48

As far as I know, all URI characters should be valid Header value
characters, so we can have an infallible constructor for Location
from an `Uri`.

This doesn't cover all uses of the Location header, since it allows
URI-references like `/People.html#tim`, but it's an ergonomic win
already, as mentioned in hyperium#48
let uri = uri.to_string();
// cf. https://www.rfc-editor.org/rfc/rfc3986#section-2
Self(
HeaderValue::from_str(&uri)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use try_from and it won't need to copy to a new buffer.

@@ -28,6 +29,18 @@ derive_header! {
name: LOCATION
}

impl Location {
/// Creates a `Location` header from a uri
pub fn uri(uri: Uri) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda wonder if we would want this function name as an accessor at some point... Not sure.

A From impl would work too.

@fasterthanlime fasterthanlime changed the title Add Location::uri constructor Add impl From<Uri> for Location + accessor Mar 15, 2022
@fasterthanlime
Copy link
Author

I addressed both comments, thanks for the feedback!

@programmerjake
Copy link

Can someone review this? I needed it today and had to use a workaround.

@kolektiv
Copy link

kolektiv commented Jul 5, 2023

Another gentle and friendly nudge - I've run up against this one too, and I'm trying to avoid a temporary fork! Absolutely understand pressures of time etc. on maintainers, I've been there! If there's something that others could help with, some of us might be able to step up?

@7sDream
Copy link

7sDream commented Dec 2, 2024

@seanmonstar
Is there any chance we can get this merge?
Location is a very common header, and current Location type is usless without this.

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.

5 participants