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

feat(headers): Add Content-Disposition header #680

Merged
merged 1 commit into from Nov 23, 2015

Conversation

mikedilger
Copy link
Contributor

fixes #561

This is fairly extensive and would benefit from some review.
It does not try to interpret language-tags (rfc 5646) but rather accepts any string (for now).

};

for section in sections {
let mut parts = section.split('=');
Copy link
Member

Choose a reason for hiding this comment

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

This should be sections.splitn(2, '='), in case any values are foo=bar=baz.

@seanmonstar
Copy link
Member

@mikedilger indeed, this is a lot of work! awesome!

I've reviewed the code, though I must say, I'm not too familiar with this header, so I may have missed something.

@mikedilger
Copy link
Contributor Author

I think I have addressed everything above except using url::percent_encoding, which depends on PR servo/rust-url#134

@seanmonstar
Copy link
Member

Seems a new version is needed of the url crate?

@mikedilger
Copy link
Contributor Author

Precisely. I'm in no hurry to get this PR merged as I can use raw headers in the meantime. But if you like things tidy, @SimonSapin would have to make a release.

@SimonSapin
Copy link
Contributor

I just published url 0.3.0 0.2.38 on crates.io which includes servo/rust-url#134. Publishing is easy but I sometimes forget to do it. Feel free to ask for it in PRs!

@seanmonstar
Copy link
Member

@mikedilger I see it's passing now. Awesome!

Could you rebase this into a single commit, feat(headers): add Content-Disposition ...? The current commits will make the changelog confusing (fixing bugs that were never released, etc).

@mikedilger
Copy link
Contributor Author

Ok, rebased onto master and collapsed into a single commit

@seanmonstar
Copy link
Member

Seems another PR updated the Cargo.toml to bump the url version already, making a conflict here.

@mikedilger
Copy link
Contributor Author

Ok, rebased again.

seanmonstar added a commit that referenced this pull request Nov 23, 2015
feat(headers): Add Content-Disposition header
@seanmonstar seanmonstar merged commit 21c4f51 into hyperium:master Nov 23, 2015
@seanmonstar
Copy link
Member

\o/

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.

Content-Disposition header
3 participants