feat(headers): Origin Header 0.9.x #853
Conversation
|
Could you rebase your commit against the 0.9.x branch? This is trying to merge all of master into 0.9.x. |
|
@seanmonstar can u take a look at this and let me know how I can make it better? |
|
Oh sorry, it doesn't send me a notification that you had rebased. Will look today. |
| /// The scheme, such as http or https | ||
| pub scheme: String, | ||
| /// The host, such as Host{hostname: "hyper.rs".to_owned(), port: None} | ||
| pub host: Host, |
There was a problem hiding this comment.
Clever.
A downside is that to set the Origin, you need to also have the Host type imported... Makes me think maybe the properties be private, with accessor methods?
There was a problem hiding this comment.
So I see the reason for a constructor, by why obstruct access?
There was a problem hiding this comment.
It might not be important yet. The idea I had was to have scheme(), hostname(), and port() accessors. By keeping the fields themselves private, it doesn't matter if we keep using Host internally, or some other field scheme.
But all the other headers are currently wide open. I'll consider a accessor pass before 1.0.
|
Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:
Guidelines are available at /~https://github.com/hyperium/hyper/blob/master/CONTRIBUTING.md This message was auto-generated by https://gitcop.com |
|
@seanmonstar could you rerun both builds? One didn't start, the other failed downloading a dependency. |
|
|
||
| impl HeaderFormat for Origin { | ||
| fn fmt_header(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| // I wanna the responsibilty of writing the host/port to Host |
There was a problem hiding this comment.
using write!(f, "{}://{}", self.scheme, self.host) should do the same.
There was a problem hiding this comment.
I originally did that, but then I get an unused result warning
There was a problem hiding this comment.
That doesn't sound right. I meant for this write! to be the only thing in the function. That way the Result is returned.
|
@untitaker whatcha think? My only last hessitation is having people build the |
|
@seanmonstar btw I still plan to obfuscate the setting if host/port via your earlier suggestion. |
| } | ||
|
|
||
| impl Origin { | ||
| pub fn new(scheme: &str, hostname: &str, port: Option<u16>) -> Origin { |
There was a problem hiding this comment.
I'd try fn new<S: Into<String>, H: Into<String>(scheme: S, hostname: H, ...). The reason is that it's probably likely that a user had to construct a String anyways to create the hostname or scheme, it's a shame to essentially have format!("{}.com", sld).as_slice().to_owned().
|
This is looking really good! 2 comments left, and we can merge! |
|
so 1, the comment about |
|
@puhrez sorry, 2 would be to remove the comment at |
|
oh haha |
Add an Origin header so users may properly send CORS requests Addresses #651
|
@seanmonstar so i think we're good here, |
Add an Origin header so users may properly send CORS requests.
Closes #651