code review 4530079: mail: new package.

mail: new package.

Basic parsing, plus date parsing.

[dsymonds, 2011-05-27 12:23:53]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-05-27 18:02:26]

http://codereview.appspot.com/4530079/diff/4001/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/4001/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:27: func ReadMessage(r io.Reader) (msg *Message, err
os.Error) {
this turned out so small & cute!

http://codereview.appspot.com/4530079/diff/4001/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:37: // TODO(dsymonds): Split specific headers out into
separate fields,
or accessor methods?

I really hate how the http package has some headers in Headers and some broken
out and removed from the map.  that means as it matures and stuff is removed
from the map that wasn't previously, old caller code breaks.

I'd like to prevent that mistake in the mail package.

(and would also like to fix http somehow)

[rsc1, 2011-05-27 18:07:49]

> I really hate how the http package has some headers in Headers and some
> broken out and removed from the map.  that means as it matures and stuff
> is removed from the map that wasn't previously, old caller code breaks.
>
> I'd like to prevent that mistake in the mail package.
>
> (and would also like to fix http somehow)

The stuff that is not in the headers in HTTP is stuff
that interacts in weird ways with the rest of the protocol.
(For example req.Host is != req.Header["Host"] necessarily.)
There's no protocol here so that shouldn't be a concern.

Russ

[bradfitz, 2011-05-27 18:25:08]

On Fri, May 27, 2011 at 11:07 AM, Russ Cox <rsc@google.com> wrote:

> > I really hate how the http package has some headers in Headers and some
> > broken out and removed from the map.  that means as it matures and stuff
> > is removed from the map that wasn't previously, old caller code breaks.
> >
> > I'd like to prevent that mistake in the mail package.
> >
> > (and would also like to fix http somehow)
>
> The stuff that is not in the headers in HTTP is stuff
> that interacts in weird ways with the rest of the protocol.
> (For example req.Host is != req.Header["Host"] necessarily.)
>

So why is Set-Cookie removed from the map?

Content-Length I can kinda understand.


> There's no protocol here so that shouldn't be a concern.


So what's the policy?  Should it all stay in the map, even if we learn to
parse it in the future?

I'd hate to have people using DKIM headers and then one day we promote them
to struct fields and their code breaks.

[rsc1, 2011-05-27 18:27:21]

>> The stuff that is not in the headers in HTTP is stuff
>> that interacts in weird ways with the rest of the protocol.
>> (For example req.Host is != req.Header["Host"] necessarily.)
>
> So why is Set-Cookie removed from the map?
> Content-Length I can kinda understand.

I don't know.  Possibly to avoid confusion, when writing
a request, about whether to use the parsed form or the
header form.  I let that slide through in the cookie
code reviews but didn't think enough about it.

Russ

[dsymonds, 2011-05-28 00:02:09]

I was thinking about extra fields for the headers that have specific
formatting, like Date, or To, as it's hard to parse those exactly
right. I can probably just add a set of methods such as
  func (m *Message) GetDate() *time.Time
  func (m *Message) GetAddrHeader(field string) []Addr
  etc.

and then there's no need for extra struct headers. It would also
preserve the original headers that you could get at if you really
wanted.


Dave.

[dsymonds, 2011-05-28 08:42:48]

I've updated the CL to include a date parsing method.

[gburd, 2011-05-28 14:53:46]

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:71: hdr := msg.Header.Get(header)
The mime package has a header parsing function, ParseMediaType.  Should date
parsing be moved to the mime package so that all header parsing functions are in
the same package?

[bradfitz, 2011-05-28 18:27:44]

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:45: // Generate layouts based on RFC 5322, section 3.3.
Awesome.

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:53: dateLayouts = make([]string, 0,
len(dows)*len(days)*len(years)*len(seconds)*len(zones))
I'd drop the capacity calculation.  It's not worth it visually for what it
saves, once at init.

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:70: func (msg *Message) HeaderAsDate(header string)
(*time.Time, os.Error) {
What are you thinking of parsing besides the "Date" header?

Maybe just call this "Date" for now and don't take a header string?

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:71: hdr := msg.Header.Get(header)
On 2011/05/28 14:53:46, gburd wrote:
> The mime package has a header parsing function, ParseMediaType.  Should date
> parsing be moved to the mime package so that all header parsing functions are
in
> the same package?  

If we do that, this would still exist and just be a wrapper, so it's not
something we have to decide now at least.

But it would be nice to have the "try all common formats" function available
from a string, because I don't think this function handles headers like this:

Received: by qwj8 with SMTP id 8so1398973qwj.4
        for <bradfitz@golang.org>; Sat, 28 May 2011 01:42:46 -0700 (PDT)

(Receives lines are half my headers....)

[dsymonds, 2011-05-29 01:12:59]

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:53: dateLayouts = make([]string, 0,
len(dows)*len(days)*len(years)*len(seconds)*len(zones))
On 2011/05/28 18:27:44, bradfitz wrote:
> I'd drop the capacity calculation.  It's not worth it visually for what it
> saves, once at init.

Yeah, okay.

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:70: func (msg *Message) HeaderAsDate(header string)
(*time.Time, os.Error) {
On 2011/05/28 18:27:44, bradfitz wrote:
> What are you thinking of parsing besides the "Date" header?
> 
> Maybe just call this "Date" for now and don't take a header string?

There's Resent-Date too. I'll also be adding address parsing, and figured that
consistency of naming and arguments would be worthwhile.

http://codereview.appspot.com/4530079/diff/3007/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:71: hdr := msg.Header.Get(header)
On 2011/05/28 14:53:46, gburd wrote:
> The mime package has a header parsing function, ParseMediaType.  Should date
> parsing be moved to the mime package so that all header parsing functions are
in
> the same package?  

Yeah, dates can occur in other places, as Brad mentioned, but also the date
parsing here is looser than a generic MIME parser might be.

[bsiegert, 2011-05-30 06:46:32]

http://codereview.appspot.com/4530079/diff/15005/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/15005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:23: // ReadMessage reads and parses a message from b.
ReadMessage reads a message from r and parses it.

[dsymonds, 2011-05-30 06:48:52]

http://codereview.appspot.com/4530079/diff/15005/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/15005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:23: // ReadMessage reads and parses a message from b.
On 2011/05/30 06:46:32, bsiegert wrote:
> ReadMessage reads a message from r and parses it.

oops.

[rsc, 2011-05-31 17:11:47]

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:18: Header textproto.MIMEHeader
In http we define our own Header type with forwarding methods
precisely so that callers don't need to know about the implementation
detail called textproto.  That's probably a good idea here too.

It would mean that you could make Date a method on Header too.

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:19: 
delete blank line?

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:23: // ReadMessage reads a message from r.
I think this needs to be expanded to point out that Body is
reading from r, so that you can't for example call ReadMessage
in a loop to populate a []*Message.  http's ReadRequest docs are
incomplete in the same way.

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:45: // Generate layouts based on RFC 5322, section 3.3.
Ouch.  Mail, the original HTTP HEAD.

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:67: // HeaderAsDate parses the named header field as a
date.
It's a little odd to make this a method on msg when
all it does is a map fetch from a field in the header.
I'd suggest

func ParseDate(date string) (*time.Time, os.Error) 

and then callers of the former msg.HeaderAsDate
can do ParseDate(msg.Header.Get("Date")) (or msg.Header.Date()).

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message_test.go
File src/pkg/mail/message_test.go (right):

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message_test.go...
src/pkg/mail/message_test.go:5: package mail_test
This makes it look like testing depends on mail,
which I really hope it does not.  Unless there is a
cycle being broken, use package mail.

[dsymonds, 2011-06-01 01:30:29]

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:18: Header textproto.MIMEHeader
On 2011/05/31 17:11:47, rsc wrote:
> In http we define our own Header type with forwarding methods
> precisely so that callers don't need to know about the implementation
> detail called textproto.  That's probably a good idea here too.
> 
> It would mean that you could make Date a method on Header too.

Done in a limited way.

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:19: 
On 2011/05/31 17:11:47, rsc wrote:
> delete blank line?

Done.

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:23: // ReadMessage reads a message from r.
On 2011/05/31 17:11:47, rsc wrote:
> I think this needs to be expanded to point out that Body is
> reading from r, so that you can't for example call ReadMessage
> in a loop to populate a []*Message.  http's ReadRequest docs are
> incomplete in the same way.

Done.

http://codereview.appspot.com/4530079/diff/14005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:67: // HeaderAsDate parses the named header field as a
date.
On 2011/05/31 17:11:47, rsc wrote:
> It's a little odd to make this a method on msg when
> all it does is a map fetch from a field in the header.
> I'd suggest
> 
> func ParseDate(date string) (*time.Time, os.Error) 
> 
> and then callers of the former msg.HeaderAsDate
> can do ParseDate(msg.Header.Get("Date")) (or msg.Header.Date()).

I don't think there's going to be much call for parsing an arbitrary string, so
I've kept parseDate private, and added a GetAsDate method that is parallel to
the generic Get method. How does this look?

[rsc, 2011-06-01 01:47:30]

It's fine to leave parseDate private until someone wants it.

http://codereview.appspot.com/4530079/diff/4/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/4/src/pkg/mail/message.go#newcode88
src/pkg/mail/message.go:88: func (h Header) GetAsDate(header string)
(*time.Time, os.Error) {
Can you make this Date and ResentDate please?
GetAs anything is just noise, and having two
separate methods means that clients don't have to
remember (or get wrong) the spelling of the string.

[dsymonds, 2011-06-01 02:49:29]

http://codereview.appspot.com/4530079/diff/4/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/4/src/pkg/mail/message.go#newcode88
src/pkg/mail/message.go:88: func (h Header) GetAsDate(header string)
(*time.Time, os.Error) {
On 2011/06/01 01:47:30, rsc wrote:
> Can you make this Date and ResentDate please?
> GetAs anything is just noise, and having two
> separate methods means that clients don't have to
> remember (or get wrong) the spelling of the string.

Resent-Date is pretty niche, so I've just made this a Date method.

[rsc, 2011-06-01 03:06:14]

> Resent-Date is pretty niche, so I've just made this a Date method.

time to publish ParseDate for the Resent-Date niche?

[rsc, 2011-06-01 03:07:56]

http://codereview.appspot.com/4530079/diff/1002/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/1002/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:78: type Header textproto.MIMEHeader
type Header map[string][]string

Don't want clients to have to learn textproto.

[dsymonds, 2011-06-01 03:29:11]

On Wed, Jun 1, 2011 at 1:06 PM, Russ Cox <rsc@golang.org> wrote:

>> Resent-Date is pretty niche, so I've just made this a Date method.
>
> time to publish ParseDate for the Resent-Date niche?

Nah, I'll wait until they ask for it. I don't think I've ever seen a
Resent-Date myself.

[dsymonds, 2011-06-01 03:29:42]

http://codereview.appspot.com/4530079/diff/1002/src/pkg/mail/message.go
File src/pkg/mail/message.go (right):

http://codereview.appspot.com/4530079/diff/1002/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:78: type Header textproto.MIMEHeader
On 2011/06/01 03:07:56, rsc wrote:
> type Header map[string][]string
> 
> Don't want clients to have to learn textproto.

Done.

[rsc, 2011-06-01 04:04:21]

LGTM

See if bradfitz has any more comments.

[bradfitz, 2011-06-01 04:08:27]

LGTM too

[dsymonds, 2011-06-01 04:10:27]

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

mail: new package.

Basic parsing, plus date parsing.

R=bradfitz, gary.burd, bsiegert, rsc
CC=golang-dev
http://codereview.appspot.com/4530079