Add charset to content type header. #83

Merged
merged 1 commit into from Aug 17, 2012

Projects

None yet

4 participants

Contributor

Useful for text/* types that contain unicode characters.

Contributor
rauchg commented Aug 17, 2012

LGTM

@rauchg rauchg and 1 other commented on an outdated diff Aug 17, 2012
lib/client.js
@@ -190,9 +190,17 @@ Client.prototype.putFile = function(src, filename, headers, fn){
fs.stat(src, function (err, stat) {
if (err) return fn(err);
+ var contentType = mime.lookup(src);
+
+ // Add charset if it's known.
+ var charset = mime.charsets.lookup(contentType, null);
+ if (charset !== null) {
rauchg
rauchg Aug 17, 2012 Contributor

We could do null != charset here.

pifantastic
pifantastic Aug 17, 2012 Contributor

Cool, fixed.

rauchg
rauchg Aug 17, 2012 Contributor

It also seems like you don't have to pass null as the second parameter since == will catch undefined as well.

pifantastic
pifantastic Aug 17, 2012 Contributor

Yeah, I was just aiming to be explicit. I can just change it to !charset if you'd prefer.

rauchg
rauchg Aug 17, 2012 Contributor

That's fine too since I guess there are no falsy charsets :P

pifantastic
pifantastic Aug 17, 2012 Contributor

I should hope not! Changed to !charset

Contributor
domenic commented Aug 17, 2012

For the record, this is what mime.charsets.lookup does:

https://github.com/broofa/node-mime/blob/master/mime.js#L97-102

Contributor
rauchg commented Aug 17, 2012

Yep I saw that, but I'll take that over performing this same check here.

Contributor
domenic commented Aug 17, 2012

Want to squash into a single commit? Happy to merge if so, then batch it up with a few other changes this weekend for the next release.

Contributor
tj commented Aug 17, 2012

github needs a squash button

Contributor

Rad, I'd never squashed before. +1 for a squash button.

Contributor
rauchg commented Aug 17, 2012

Can you break it down into smaller atomic commits?

Contributor
rauchg commented Aug 17, 2012

Just kidding :P

@rauchg rauchg merged commit c34b77e into Automattic:master Aug 17, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment