Skip to content
This repository has been archived by the owner on Dec 14, 2023. It is now read-only.

Add hyper::Uri support #21

Closed
wants to merge 4 commits into from
Closed

Add hyper::Uri support #21

wants to merge 4 commits into from

Conversation

ferranpujolcamins
Copy link

Based on PR #20

Eijebong and others added 3 commits September 16, 2017 10:57
@ferranpujolcamins ferranpujolcamins changed the title Add uri support Add hyper::Uri support Oct 29, 2017
@DarrenTsung
Copy link

Is there anything blocking this PR from landing? Happy to help with any changes needed.

Copy link
Contributor

@nox nox left a comment

Choose a reason for hiding this comment

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

Those dependency bumps require a new major version for this crate.

serde = "1.0"
serde_bytes = "0.10"
time = "0.1"

[dev-dependencies]
serde_test = "1.0"
time = "0.1"

[replace]
"hyper:0.11.2" = { git = "https://github.com/hyperium/hyper.git" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this here?

@@ -412,7 +415,7 @@ impl<'a> Serialize for Ser<'a, Headers> {
for header in self.v.iter() {
let name = header.name();
let value = self.v.get_raw(name).unwrap();
serializer.serialize_entry(name, &Value(value, self.pretty))?;
serializer.serialize_entry(name, &Value(&value.iter().map(|v| v.to_vec()).collect::<Vec<Vec<u8>>>(), self.pretty))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to allocate a new Vec<Vec<u8>>?

{
Uri::from_str(v)
.map(De::new)
.map_err(|e| E::custom(format!("{:?}", e)))
Copy link
Contributor

Choose a reason for hiding this comment

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

That shouldn't use Debug.

fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where S: Serializer,
{
serializer.serialize_str(&self.v.to_string())
Copy link
Contributor

Choose a reason for hiding this comment

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

This allocates for nothing.

@@ -17,12 +17,15 @@ doctest = false

[dependencies]
cookie = {version = "0.6", default-features = false}
hyper = "0.10"
mime = "0.2"
hyper = { version = "0.11", features = ["raw_status"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is raw_status needed?

@DarrenTsung
Copy link

I opened another PR from my fork integrating the feedback, see #23.

@ferranpujolcamins
Copy link
Author

This PR was erased from my memory. Thanks @DarrenTsung for picking it up!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants