code review 4588041: mail: use strconv.QuoteToASCII for formatting addresses.

mail: format addresseses correctly.

Also remove an obsolete TODO while I'm here.

[dsymonds, 2011-06-07 12:28:51]

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

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

[r, 2011-06-07 12:30:59]

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

http://codereview.appspot.com/4588041/diff/4001/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:142: return "\"" + strconv.QuoteToASCII(a.Name) + "\" "
+ s
is that really what you want? if my name is in japanese, this will turn it into
"\u5642".

[rsc, 2011-06-07 19:57:13]

This is almost certainly not what you want.

[rsc, 2011-06-07 19:58:55]

RFC 2047 might be what you're looking for.

[dsymonds, 2011-06-08 01:49:36]

This is ready for another look.

It tries to format the address minimally (quoted-string if possible,
falling back to RFC 2047 encoded-word).

[rsc, 2011-06-08 02:08:29]

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

http://codereview.appspot.com/4588041/diff/8002/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:137: // String formats the address as a valid RFC 5322
address.
I'm not 100% this should be the String method.
I think you want one method for human-readable (%q <%s>) and one for machine
encoding (RFC).  Usually String means
the former.  Maybe this is RFCString?  EncodedString?

If you're printing "I got mail from %s" you want the real UTF-8 not the escapes.

http://codereview.appspot.com/4588041/diff/8002/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:165: n := "=?utf-8?q?"
please print into a bytes.Buffer
the repeated += is quadratic in garbage generated.
also, if you loop over the bytes in the name the
printing is easier since you don't have to re-encode
the runes.

[dsymonds, 2011-06-08 02:48:15]

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

http://codereview.appspot.com/4588041/diff/8002/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:137: // String formats the address as a valid RFC 5322
address.
On 2011/06/08 02:08:30, rsc wrote:
> I'm not 100% this should be the String method.
> I think you want one method for human-readable (%q <%s>) and one for machine
> encoding (RFC).  Usually String means
> the former.  Maybe this is RFCString?  EncodedString?
> 
> If you're printing "I got mail from %s" you want the real UTF-8 not the
escapes.

Well, if you're doing that, you should just use addr.Address; you probably don't
want the display name either.

If this were RFCString then there'd probably be no need for a String method,
because if you care about the formatting then you're either going to use
RFCString or you'd access the fields directly.

I'm happy to rename this if that's the consensus, but I'd be wary of having
something close-to-compliant be mistaken for compliant (as on your %q <%s>
suggestion).

http://codereview.appspot.com/4588041/diff/8002/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:165: n := "=?utf-8?q?"
On 2011/06/08 02:08:30, rsc wrote:
> please print into a bytes.Buffer
> the repeated += is quadratic in garbage generated.
> also, if you loop over the bytes in the name the
> printing is easier since you don't have to re-encode
> the runes.
> 

Done.

[rsc, 2011-06-08 03:00:32]

> I'm happy to rename this if that's the consensus, but I'd be wary of
> having something close-to-compliant be mistaken for compliant (as on
> your %q <%s> suggestion).

I'm definitely wary of that too.  Let's go with String = RFC output
for now and see whether we reconsider.

[rsc, 2011-06-08 03:02:59]

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

http://codereview.appspot.com/4588041/diff/16001/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:147: for _, c := range a.Name {
All these loops might as well range over bytes
(for i := 0; i < len(a.Name); i++).
The quoting loop below especially so.
(I mentioned that one last time.)

[dsymonds, 2011-06-08 03:12:49]

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

http://codereview.appspot.com/4588041/diff/16001/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:147: for _, c := range a.Name {
On 2011/06/08 03:03:00, rsc wrote:
> All these loops might as well range over bytes
> (for i := 0; i < len(a.Name); i++).
> The quoting loop below especially so.
> (I mentioned that one last time.)
> 

Done.

[rsc, 2011-06-08 03:15:13]

LGTM

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

http://codereview.appspot.com/4588041/diff/17005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:160: return "\"" + b.String() + "\" " + s
might as well use the bytes.Buffer to eliminate these + too

http://codereview.appspot.com/4588041/diff/17005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:177: return b.String() + " " + s
same

[r, 2011-06-08 03:19:48]

oh - i see - you didn't see my comment because it was stuck in a browser
somewhere.

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

http://codereview.appspot.com/4588041/diff/8002/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:437: return '!' <= c && c <= '~' && c != '\\' && c !=
'"'
hard to read
if c == '\\' || c == ' ' {
  return true
}
return isVchar(c)   or just inline it.

http://codereview.appspot.com/4588041/diff/17005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:431: return bytes.IndexByte(atextChars, c) >= 0
you didn't respond to my suggestion here, or at least i missed your response.

[dsymonds, 2011-06-08 03:20:43]

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

http://codereview.appspot.com/4588041/diff/17005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:160: return "\"" + b.String() + "\" " + s
On 2011/06/08 03:15:13, rsc wrote:
> might as well use the bytes.Buffer to eliminate these + too

Done.

http://codereview.appspot.com/4588041/diff/17005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:177: return b.String() + " " + s
On 2011/06/08 03:15:13, rsc wrote:
> same

Done.

[dsymonds, 2011-06-08 03:26:35]

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

http://codereview.appspot.com/4588041/diff/8002/src/pkg/mail/message.go#newco...
src/pkg/mail/message.go:437: return '!' <= c && c <= '~' && c != '\\' && c !=
'"'
On 2011/06/08 03:19:48, r wrote:
> hard to read
> if c == '\\' || c == ' ' {
>   return true
> }
> return isVchar(c)   or just inline it.

Your logic is inverted, but done.

http://codereview.appspot.com/4588041/diff/17005/src/pkg/mail/message.go#newc...
src/pkg/mail/message.go:431: return bytes.IndexByte(atextChars, c) >= 0
On 2011/06/08 03:19:48, r wrote:
> you didn't respond to my suggestion here, or at least i missed your response.

I can't find your previous suggestion. Was it what you made on isAtext regarding
using bytes.IndexByte? If so, it'd be a lot more tedious (and error prone) to
write out all the values, and doing it in an init function feels overkill just
to avoid a few comparisons.

[r, 2011-06-08 03:31:19]

LGTM

[dsymonds, 2011-06-08 03:32:59]

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

mail: format addresseses correctly.

Also remove an obsolete TODO while I'm here.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/4588041