code review 186160: mime: new package, use in http

mime: new package, use in http

[hoisie, 2010-01-15 00:21:52]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2010-01-15 22:20:56]

http://codereview.appspot.com/186160/diff/23/25
File src/pkg/http/mimetype.go (right):

http://codereview.appspot.com/186160/diff/23/25#newcode5
src/pkg/http/mimetype.go:5: // mime type association
blank line after this comment.
where it is now it will get used as a package-level
doc comment.

http://codereview.appspot.com/186160/diff/23/25#newcode15
src/pkg/http/mimetype.go:15: var defaulttypes = map[string]string{}
should be mixed case throughout

defaultTypes
typeFiles
commonTypes
etc

http://codereview.appspot.com/186160/diff/23/25#newcode23
src/pkg/http/mimetype.go:23: var commontypes = map[string]string{
no need for two maps.  start with this one and
let the other one add entries.

then the single map can be called mimeTypes.

http://codereview.appspot.com/186160/diff/23/25#newcode35
src/pkg/http/mimetype.go:35: func fileexists(filename string) bool {
Unnecessary; just open the file and read it.

http://codereview.appspot.com/186160/diff/23/25#newcode48
src/pkg/http/mimetype.go:48: if !fileexists(filename) {
don't require reading the whole file into memory.
use os.Open and bufio.NewReader and then read it line at a time.

http://codereview.appspot.com/186160/diff/23/25#newcode61
src/pkg/http/mimetype.go:61: fields := strings.Fields(line)
probably have to cut # comments.

http://codereview.appspot.com/186160/diff/23/25#newcode66
src/pkg/http/mimetype.go:66: typename := fields[0]
if strings.HasPrefix(typename, "text/") {
    typename += "; charset=utf-8"
}

http://codereview.appspot.com/186160/diff/23/25#newcode74
src/pkg/http/mimetype.go:74: func init() {
please use once.Do instead of init.
this table is not fundamental to http
and can be delayed until it is needed.

http://codereview.appspot.com/186160/diff/23/25#newcode80
src/pkg/http/mimetype.go:80: // returns the MIME type given a file extension
mimeType returns ...

http://codereview.appspot.com/186160/diff/23/25#newcode82
src/pkg/http/mimetype.go:82: 
delete blank line

http://codereview.appspot.com/186160/diff/23/25#newcode89
src/pkg/http/mimetype.go:89: } else if typ, ok := commontypes[ext]; ok {
no need for else; you just returned.

[hoisie, 2010-01-21 01:27:46]

Done

On 2010/01/15 22:20:56, rsc wrote:
> http://codereview.appspot.com/186160/diff/23/25
> File src/pkg/http/mimetype.go (right):
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode5
> src/pkg/http/mimetype.go:5: // mime type association
> blank line after this comment.
> where it is now it will get used as a package-level
> doc comment.
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode15
> src/pkg/http/mimetype.go:15: var defaulttypes = map[string]string{}
> should be mixed case throughout
> 
> defaultTypes
> typeFiles
> commonTypes
> etc
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode23
> src/pkg/http/mimetype.go:23: var commontypes = map[string]string{
> no need for two maps.  start with this one and
> let the other one add entries.
> 
> then the single map can be called mimeTypes.
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode35
> src/pkg/http/mimetype.go:35: func fileexists(filename string) bool {
> Unnecessary; just open the file and read it.
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode48
> src/pkg/http/mimetype.go:48: if !fileexists(filename) {
> don't require reading the whole file into memory.
> use os.Open and bufio.NewReader and then read it line at a time.
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode61
> src/pkg/http/mimetype.go:61: fields := strings.Fields(line)
> probably have to cut # comments.
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode66
> src/pkg/http/mimetype.go:66: typename := fields[0]
> if strings.HasPrefix(typename, "text/") {
>     typename += "; charset=utf-8"
> }
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode74
> src/pkg/http/mimetype.go:74: func init() {
> please use once.Do instead of init.
> this table is not fundamental to http
> and can be delayed until it is needed.
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode80
> src/pkg/http/mimetype.go:80: // returns the MIME type given a file extension
> mimeType returns ...
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode82
> src/pkg/http/mimetype.go:82: 
> delete blank line
> 
> http://codereview.appspot.com/186160/diff/23/25#newcode89
> src/pkg/http/mimetype.go:89: } else if typ, ok := commontypes[ext]; ok {
> no need for else; you just returned.

[rsc, 2010-01-26 20:46:21]

LGTM

but please make the few small changes below.

http://codereview.appspot.com/186160/diff/2002/31
File src/pkg/http/mimetype.go (right):

http://codereview.appspot.com/186160/diff/2002/31#newcode34
src/pkg/http/mimetype.go:34: func loadMimeFile(filename string) {
I'd return a bool here that says whether the file was loaded.
Then you can break out of the loop in initMime once one succeeds.

http://codereview.appspot.com/186160/diff/2002/31#newcode38
src/pkg/http/mimetype.go:38: return
return false

http://codereview.appspot.com/186160/diff/2002/31#newcode47
src/pkg/http/mimetype.go:47: return
break

http://codereview.appspot.com/186160/diff/2002/31#newcode58
src/pkg/http/mimetype.go:58: for _, ext := range (fields[1:]) {
drop parens

http://codereview.appspot.com/186160/diff/2002/31#newcode65
src/pkg/http/mimetype.go:65: }
return true

http://codereview.appspot.com/186160/diff/2002/31#newcode68
src/pkg/http/mimetype.go:68: for _, filename := range (typeFiles) {
drop parens

[hoisie, 2010-01-26 22:51:43]

ok done

On 2010/01/26 20:46:21, rsc wrote:
> LGTM
> 
> but please make the few small changes below.
> 
> http://codereview.appspot.com/186160/diff/2002/31
> File src/pkg/http/mimetype.go (right):
> 
> http://codereview.appspot.com/186160/diff/2002/31#newcode34
> src/pkg/http/mimetype.go:34: func loadMimeFile(filename string) {
> I'd return a bool here that says whether the file was loaded.
> Then you can break out of the loop in initMime once one succeeds.
> 
> http://codereview.appspot.com/186160/diff/2002/31#newcode38
> src/pkg/http/mimetype.go:38: return
> return false
> 
> http://codereview.appspot.com/186160/diff/2002/31#newcode47
> src/pkg/http/mimetype.go:47: return
> break
> 
> http://codereview.appspot.com/186160/diff/2002/31#newcode58
> src/pkg/http/mimetype.go:58: for _, ext := range (fields[1:]) {
> drop parens
> 
> http://codereview.appspot.com/186160/diff/2002/31#newcode65
> src/pkg/http/mimetype.go:65: }
> return true
> 
> http://codereview.appspot.com/186160/diff/2002/31#newcode68
> src/pkg/http/mimetype.go:68: for _, filename := range (typeFiles) {
> drop parens

[hoisie, 2010-01-27 06:23:06]

Would you like to just put this into a separate package? One of my projects
would find this very useful, but I wouldn't have access to it as-is. 

On 2010/01/26 22:51:43, hoisie wrote:
> ok done
> 
> On 2010/01/26 20:46:21, rsc wrote:
> > LGTM
> > 
> > but please make the few small changes below.
> > 
> > http://codereview.appspot.com/186160/diff/2002/31
> > File src/pkg/http/mimetype.go (right):
> > 
> > http://codereview.appspot.com/186160/diff/2002/31#newcode34
> > src/pkg/http/mimetype.go:34: func loadMimeFile(filename string) {
> > I'd return a bool here that says whether the file was loaded.
> > Then you can break out of the loop in initMime once one succeeds.
> > 
> > http://codereview.appspot.com/186160/diff/2002/31#newcode38
> > src/pkg/http/mimetype.go:38: return
> > return false
> > 
> > http://codereview.appspot.com/186160/diff/2002/31#newcode47
> > src/pkg/http/mimetype.go:47: return
> > break
> > 
> > http://codereview.appspot.com/186160/diff/2002/31#newcode58
> > src/pkg/http/mimetype.go:58: for _, ext := range (fields[1:]) {
> > drop parens
> > 
> > http://codereview.appspot.com/186160/diff/2002/31#newcode65
> > src/pkg/http/mimetype.go:65: }
> > return true
> > 
> > http://codereview.appspot.com/186160/diff/2002/31#newcode68
> > src/pkg/http/mimetype.go:68: for _, filename := range (typeFiles) {
> > drop parens

[rsc1, 2010-01-28 23:56:22]

sure.  the mimetype.go file looks good.
how about moving it to mime/type.go
and then make the function it exports
mime.TypeByExtension(ext string)?

i'll wait for you to update the CL.
please hg sync before re-mailing.

[hoisie, 2010-02-01 22:39:02]

Ok done

On 2010/01/28 23:56:22, rsc1 wrote:
> sure.  the mimetype.go file looks good.
> how about moving it to mime/type.go
> and then make the function it exports
> mime.TypeByExtension(ext string)?
> 
> i'll wait for you to update the CL.
> please hg sync before re-mailing.

[rsc, 2010-02-04 07:07:37]

please make the CL description something like

mime: new package, use in http

http://codereview.appspot.com/186160/diff/6001/6005
File src/pkg/mime/mime_test.go (right):

http://codereview.appspot.com/186160/diff/6001/6005#newcode1
src/pkg/mime/mime_test.go:1: // Copyright 2009 The Go Authors. All rights
reserved.
2010

http://codereview.appspot.com/186160/diff/6001/6005#newcode10
src/pkg/mime/mime_test.go:10: "testing"
turn into a one-line import

http://codereview.appspot.com/186160/diff/6001/6005#newcode21
src/pkg/mime/mime_test.go:21: for ext, exp := range (typeTests) {
drop parens around range argument

exp, want

ext and exp are too similar

http://codereview.appspot.com/186160/diff/6001/6005#newcode24
src/pkg/mime/mime_test.go:24: t.Errorf("TypeByExtension expected %q got %q",
exp, val)
t.Errorf("TypeByExtension(%q) = %q, want %q", ext, val, want)

http://codereview.appspot.com/186160/diff/6001/6007
File src/pkg/mime/type.go (right):

http://codereview.appspot.com/186160/diff/6001/6007#newcode1
src/pkg/mime/type.go:1: // Copyright 2009 The Go Authors. All rights reserved.
2010

http://codereview.appspot.com/186160/diff/6001/6007#newcode5
src/pkg/mime/type.go:5: // mime type association
Should be a proper package comment, even if it's a short one.
http://golang.org/doc/effective_go.html#commentary

// The mime package translates file name extensions to MIME types.
// It consults the local system's mime.types file, which must be installed
// under one of these names:
//
//   /etc/mime.types
//   /etc/apache2/mime.types
//   /etc/apache/mime.types
//
package mime

http://codereview.appspot.com/186160/diff/6001/6007#newcode36
src/pkg/mime/type.go:36: 
drop blank line; test should be next to the thing it is testing.
you've got a blank line after every statement in this function.

http://codereview.appspot.com/186160/diff/6001/6007#newcode58
src/pkg/mime/type.go:58: for _, ext := range (fields[1:]) {
drop parens around range argument

http://codereview.appspot.com/186160/diff/6001/6007#newcode68
src/pkg/mime/type.go:68: for _, filename := range (typeFiles) {
drop parens around range argument.

http://codereview.appspot.com/186160/diff/6001/6007#newcode73
src/pkg/mime/type.go:73: // returns the MIME type given a file extension
see link above; should be a complete English sentence: this is documentation
that will be shown to the programmer.

// TypeByExtension returns the MIME type associated with the file extension ext.
// The extension ext should begin with a leading dot, as in ".html". 
// When ext has no associated type, TypeByExtension returns "".

http://codereview.appspot.com/186160/diff/6001/6007#newcode77
src/pkg/mime/type.go:77: if strings.HasPrefix(ext, ".") {
It bothers me to have two ways to call the function.
Pick one and callers that are wrong will quickly figure that out.
I suggest requiring the dot, as in the comment above,
because that works nicely with path.Ext (see http/fs.go).
I also suggest adding the dot prefix when loading mime.types,
so that this function is then:

once.Do(initMime)
typ, _ := mimeTypes[ext]
return typ

[hoisie, 2010-02-04 17:24:00]

done

[hoisie, 2010-02-09 01:51:15]

any update on this?

[rsc, 2010-02-09 21:25:43]

looks good; one small bug

http://codereview.appspot.com/186160/diff/9008/8015
File src/pkg/mime/mime_test.go (right):

http://codereview.appspot.com/186160/diff/9008/8015#newcode13
src/pkg/mime/mime_test.go:13: ".t2": "text/test; charset=utf-8",
add a test for ".png"
to catch bug in other file.

http://codereview.appspot.com/186160/diff/9008/8015#newcode17
src/pkg/mime/mime_test.go:17: loadMimeFile("test.types")
change to

typeFiles = []string{"test.types"}

and let TypeByExtension load the file.

http://codereview.appspot.com/186160/diff/9008/8017
File src/pkg/mime/type.go (right):

http://codereview.appspot.com/186160/diff/9008/8017#newcode29
src/pkg/mime/type.go:29: "css": "text/css",
need dots on all these

[hoisie, 2010-02-09 22:16:59]

Done. Nice catch, btw

On 2010/02/09 21:25:43, rsc wrote:
> looks good; one small bug
> 
> http://codereview.appspot.com/186160/diff/9008/8015
> File src/pkg/mime/mime_test.go (right):
> 
> http://codereview.appspot.com/186160/diff/9008/8015#newcode13
> src/pkg/mime/mime_test.go:13: ".t2": "text/test; charset=utf-8",
> add a test for ".png"
> to catch bug in other file.
> 
> http://codereview.appspot.com/186160/diff/9008/8015#newcode17
> src/pkg/mime/mime_test.go:17: loadMimeFile("test.types")
> change to
> 
> typeFiles = []string{"test.types"}
> 
> and let TypeByExtension load the file.
> 
> http://codereview.appspot.com/186160/diff/9008/8017
> File src/pkg/mime/type.go (right):
> 
> http://codereview.appspot.com/186160/diff/9008/8017#newcode29
> src/pkg/mime/type.go:29: "css": "text/css",
> need dots on all these

[rsc, 2010-02-10 04:40:21]

LGTM

thanks

[rsc, 2010-02-10 04:47:50]

*** Submitted as http://code.google.com/p/go/source/detail?r=f1f1151848c3 ***

mime: new package, use in http

R=rsc
CC=golang-dev
http://codereview.appspot.com/186160

Committer: Russ Cox <rsc@golang.org>