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

Define `HEADER_FIELD_ENCODE_SET` #428

Closed
wants to merge 1 commit into from
Closed

Conversation

@Ralith
Copy link

Ralith commented Jan 20, 2018

This is needed for encoding UTF-8 in header fields, e.g. Content-Disposition: attachment; filename*=UTF-8''.... Defined in RFC8187. I'd appreciate a double-check that I got the characters right.


This change is Reviewable

@Ralith Ralith force-pushed the Ralith:header-fields branch from 4289d7a to 49ab1ba Jan 20, 2018
@SimonSapin
Copy link
Member

SimonSapin commented Jan 20, 2018

The define_encode_set! macro is intended to be usable in other crates. Is there a reason this specific set should be defined in this crate?

@Ralith
Copy link
Author

Ralith commented Jan 21, 2018

Because it's a standardized encoding commonly needed by both web services and clients, similar to the others. Correct implementations of the Content-Disposition header are a wonderful thing.

@SimonSapin
Copy link
Member

SimonSapin commented Jan 21, 2018

Still, wouldn’t this belong more in a crate like http? Even in things with RFCs there are many that use precent-encoding slightly differently, I’d prefer this crate not to be responsible for them all.

@Ralith
Copy link
Author

Ralith commented Jan 21, 2018

The http crate does not depend on this crate. Is that likely to change?

@SimonSapin
Copy link
Member

SimonSapin commented Jul 15, 2019

Closing as out of scope. Even with this PR, this crate does not provide an implementation (correct or not) of the Content-Disposition header.

@SimonSapin SimonSapin closed this Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.