-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(headers): support Opaque origin headers #1147
feat(headers): support Opaque origin headers #1147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work! Just a slight tweak can rid us of the allow(unused_variables)
attributes, and we're good to go!
src/header/common/origin.rs
Outdated
pub fn scheme(&self) -> &str { | ||
&(self.scheme) | ||
#[allow(unused_variables)] | ||
// Variable `host` is considered unused, although there does not seem to be a way to omit it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just elide all the fields you don't want, like so OriginOrNull::Origin { ref scheme, .. }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is really neat, thanks for pointing it out! I am a bit new to Rust and didn't know about this until now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another trick is to add _
to the names of otherwise-unused variables, which suppresses the warning.
src/header/common/origin.rs
Outdated
&(self.host) | ||
#[allow(unused_variables)] | ||
// Variable `scheme` is considered unused, although there does not seem to be a way to omit it here. | ||
pub fn host(&self) -> Option<&Host> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aw, having to use Option
s is unfortunate, that I had forgotten about. Oh well, there's no other way!
Add support for Opaque origin header, serializing it to `null`. https://html.spec.whatwg.org/multipage/browsers.html#concept-origin Closes hyperium#1065
0b56c7f
to
c0632cc
Compare
I went ahead and squashed the extra commit as well as force pushed it. Not sure if that's the right approach here, but I wanted to keep this change as a single commit. Let me know if there are other things I can add/improve here. Thank you! |
Thanks! Excellent work! |
Add support for Opaque origin header, serializing it to `null`. https://html.spec.whatwg.org/multipage/browsers.html#concept-origin Closes hyperium#1065 BREAKING CHANGE: `Origin.scheme` and `Origin.host` now return `Option`s, since the `Origin` could be `null`.
Add support for Opaque origin header, serializing it to
null
.https://html.spec.whatwg.org/multipage/browsers.html#concept-origin
Closes #1065