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 AcceptCharset header #300

Closed

Conversation

hugoduncan
Copy link
Contributor

Adds support for the Accept-Charset header. Encodes the charset as a string.

Adds support for the Accept-Charset header.  Encodes the charset as a
string.
@pyfisch
Copy link
Contributor

pyfisch commented Feb 7, 2015

Looks good to me, although I would prefer a stronger typed charset type.

@hugoduncan
Copy link
Contributor Author

I am happy to add an enum for the charset type, but I think we would need to agree which list of charsets to include (and have the capability of specifying a string). Would the list in the "Preferred MIME Name" column of the IANA rgistry (as suggested by rfc2616) be suitable?

@seanmonstar
Copy link
Member

I'd prefer stronger typed also, but that list is quite long. Can either keep as String for now, or try an enum with 5-10 of the most common, and then a Ext(String) variant.

@pyfisch
Copy link
Contributor

pyfisch commented Feb 7, 2015

Actually the list is not that long. It can be a separate crate like mime.rs

@hugoduncan
Copy link
Contributor Author

The suggested list has 24 entries, which seems reasonable to me. @pyfisch Would you be interested in owning such a crate (seems it would go well alongside rust-language-tags)? @seanmonstar would you be OK adding such a dependency?

@reem
Copy link
Contributor

reem commented Feb 7, 2015

24 entries is short enough to live in hyper imo, but I wouldn't mind another crate in hyperium to help split things up.

@seanmonstar
Copy link
Member

24 can fit in hyper. I made mime a separate crate as it seemed useful for others.

Make Charset more strongly typed.
@hugoduncan
Copy link
Contributor Author

@seanmonstar
Copy link
Member

Oh nice. That'd be rad if we could integrate that, since we already depend on encoding (via rust-url). @lifthrasiir thoughts about a type that we could use in the headers? A label?

@lifthrasiir
Copy link

@seanmonstar lifthrasiir/rust-encoding#64 is probably relevant. I'm not sure whether this is truly desirable or not though.

@seanmonstar
Copy link
Member

@lifthrasiir yep, that sounds exactly what would be best here...

"ISO-8859-8" => Iso_8859_8,
"ISO-8859-9" => Iso_8859_9,
"ISO-8859-10" => Iso_8859_10,
"Shift-JIS" => Shift_Jis,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't that be "SHIFT-JIS" since you're using uppercase?

@seanmonstar
Copy link
Member

merged manually

@seanmonstar seanmonstar closed this Mar 4, 2015
@sbstp
Copy link
Contributor

sbstp commented Mar 4, 2015

I'm pretty sure the Shift-JIS bug is still there.
I added a test, assert_eq!(Shift_Jis,"Shift-JIS".parse().unwrap()); and it doesn't work.
The line "Shift-JIS" => Shift_Jis, never matches and it goes straight to Ext("SHIFT-JIS").

@seanmonstar
Copy link
Member

@sbstp oh! i see what you mean, since to_ascii_uppercase() is called just before it. I was quite confused at first, since it's Shift-JIS in both from_str and name.

@sbstp
Copy link
Contributor

sbstp commented Mar 4, 2015

Do you want me to PR a fix?

@seanmonstar
Copy link
Member

Sure. Though, if the name is supposed to mixed case like that, then the fix
must be about using to_uppercase. If it's not supposed to, then your
suggestion works perfectly.

On Tue, Mar 3, 2015, 9:15 PM Simon Bernier St-Pierre <
notifications@github.com> wrote:

Do you want me to PR a fix?


Reply to this email directly or view it on GitHub
#300 (comment).

@sbstp
Copy link
Contributor

sbstp commented Mar 4, 2015

I was only going to fix the parsing part, since uppercase should handle mixed case.

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.

6 participants