code review 808041: The nntp package implements a client for the news proto...

The nntp package implements a client for the news protocol NNTP,
as defined in RFC 3977.

[cemeyer, 2010-03-28 00:31:13]

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

I'd like you to review this change.

[rsc, 2010-03-28 07:36:28]

This looks like a good start.
I think it can probably be about 30% shorter, though.
I've suggested some refactorings and API changes below.
The main one is the use of io.Reader instead of trying
to store message bodies in memory.

Obviously there's some duplication here with HTTP and
with the eventual SMTP, POP3, IMAP4, etc libraries.
I feel like I saw a general library for "text style
internet protocols" that abstracted some of this stuff
once, but I don't remember where or even what language
it was for.  Do you happen to know what I'm talking about?
It seems like it would be worth looking at for
inspiration if we wanted to factor some of the common
bits out into a common package like net/textproto or
something.

Thanks for working on this.

http://codereview.appspot.com/808041/diff/2001/3002
File src/pkg/nntp/Makefile (right):

http://codereview.appspot.com/808041/diff/2001/3002#newcode5
src/pkg/nntp/Makefile:5: include ../../../src/Make.$(GOARCH)
include ../../Make.$(GOARCH)

http://codereview.appspot.com/808041/diff/2001/3002#newcode11
src/pkg/nntp/Makefile:11: include ../../../src/Make.pkg
include ../../Make.pkg

http://codereview.appspot.com/808041/diff/2001/3003
File src/pkg/nntp/nntp.go (right):

http://codereview.appspot.com/808041/diff/2001/3003#newcode5
src/pkg/nntp/nntp.go:5: // This package provides a programmatic interface for
NNTP clients.
"programmatic" is redundant in most programs.

// The nntp package implements a client for the news protocol NNTP,
// as defined in RFC 3977.

http://codereview.appspot.com/808041/diff/2001/3003#newcode25
src/pkg/nntp/nntp.go:25: // Represents an error message from the NNTP server.
Doc comments should be full sentences.
See http://golang.org/doc/effective_go.html#commentary

http://codereview.appspot.com/808041/diff/2001/3003#newcode26
src/pkg/nntp/nntp.go:26: type Error struct {
This error seems to serve multiple purposes:
it represents more than just an error message 
from the NNTP server.  Maybe it should go back
to being just an NNTP server error and the other
cases should be some other kind of error, like

type ProtocolError string

http://codereview.appspot.com/808041/diff/2001/3003#newcode33
src/pkg/nntp/nntp.go:33: realsock   net.Conn
Please avoid the parochial term socket.
The rest of the network libraries do not use it.

conn net.Conn
r *bufio.Reader

would be fine.

http://codereview.appspot.com/808041/diff/2001/3003#newcode35
src/pkg/nntp/nntp.go:35: user, pass string
Can avoid storing these in the struct.

http://codereview.appspot.com/808041/diff/2001/3003#newcode36
src/pkg/nntp/nntp.go:36: noposting  bool
Avoid double negative: canPost bool

http://codereview.appspot.com/808041/diff/2001/3003#newcode40
src/pkg/nntp/nntp.go:40: type nntpResponse struct {
The nntp here is redundant.  type response struct {
But maybe the struct isn't needed either.  (See below.)

http://codereview.appspot.com/808041/diff/2001/3003#newcode46
src/pkg/nntp/nntp.go:46: // The status of a group named 'Name'. High and Low
indicate the highest and lowest
Can drop quotes around Name.
Quotes on next line should be "" not '' (they are strings).

http://codereview.appspot.com/808041/diff/2001/3003#newcode49
src/pkg/nntp/nntp.go:49: type GroupStatus struct {
There doesn't appear to be a Group, so GroupStatus seems
unnecessarily verbose.

http://codereview.appspot.com/808041/diff/2001/3003#newcode57
src/pkg/nntp/nntp.go:57: From    string
Not sure why these are pulled out of the header map.

http://codereview.appspot.com/808041/diff/2001/3003#newcode60
src/pkg/nntp/nntp.go:60: Headers map[string][]string
More common (for example, in http.Request) to be singular:
Group []string, Header map[string][]string

http://codereview.appspot.com/808041/diff/2001/3003#newcode65
src/pkg/nntp/nntp.go:65: *Header
You can embed a Header directly without the *.
That's probably better here.

Or maybe these should be one type, Article,
and a header-only article has Body == nil.

http://codereview.appspot.com/808041/diff/2001/3003#newcode66
src/pkg/nntp/nntp.go:66: Body string
Large data block such as article bodies probably
make more sense as []byte than as string.
Even better for large bodies, it might make
sense for Body to be an io.Reader, and require
the caller to read what it wants of the article
before it issues the next command.
I've continued to sketch out this latter approach below.

http://codereview.appspot.com/808041/diff/2001/3003#newcode69
src/pkg/nntp/nntp.go:69: func (n *Header) String() string {
String should return something short, usually on
a single line.  For example, String() might be
[NNTP article <message-id>].

The functionality here  should be something like

func (h *Header) WriteTo(w io.Writer) os.Error 

which won't need to construct a giant string.
Also, WriteTo should use the standard Go newline
convention \n instead of \r\n.

http://codereview.appspot.com/808041/diff/2001/3003#newcode80
src/pkg/nntp/nntp.go:80: func (n *Article) String() string {
Same comment about String.

http://codereview.appspot.com/808041/diff/2001/3003#newcode89
src/pkg/nntp/nntp.go:89: if n.Code == 1000 {
This should be a different kind of error.
Otherwise clients have to know that sometimes
n.Code == 1000 to mean something completely 
different.

http://codereview.appspot.com/808041/diff/2001/3003#newcode96
src/pkg/nntp/nntp.go:96: typeStr = "Informative message"
I don't think these strings add much, and they make the function 40 lines long. 
I suggest instead:

return fmt.Sprintf("%d %s", n.Code, n.Msg)

http://codereview.appspot.com/808041/diff/2001/3003#newcode156
src/pkg/nntp/nntp.go:156: func Connect(host, username, password string) (*Conn,
os.Error) {
In the Go libraries this is typically called Dial, not Connect.

http://codereview.appspot.com/808041/diff/2001/3003#newcode187
src/pkg/nntp/nntp.go:187: func (n *Conn) Authenticate() os.Error {
Does this need to be public?  It looks like no.
Alternately, maybe it should stay public, take over
the username and password arguments from Dial,
and not be called by default - not all NNTP servers
need a user name and password.

http://codereview.appspot.com/808041/diff/2001/3003#newcode187
src/pkg/nntp/nntp.go:187: func (n *Conn) Authenticate() os.Error {
n is an odd name for a *Conn.  c?

http://codereview.appspot.com/808041/diff/2001/3003#newcode209
src/pkg/nntp/nntp.go:209: line = line + "\r\n"
return fmt.Fprintf(n.conn, "%s\r\n", line)

Or, better just call that directly where this function is used.

http://codereview.appspot.com/808041/diff/2001/3003#newcode218
src/pkg/nntp/nntp.go:218: if err == io.ErrShortWrite {
Short writes only happen if the writer cannot write anymore.
(It's not like non-blocking network i/o where the writer just
doesn't feel like writing anymore but might change its mind.)
So this loop isn't necessary.

http://codereview.appspot.com/808041/diff/2001/3003#newcode230
src/pkg/nntp/nntp.go:230: res, err := n.sock.ReadString('\n')
return c.r.ReadString('\n')

Or, better, just call that directly where this function is used.

http://codereview.appspot.com/808041/diff/2001/3003#newcode238
src/pkg/nntp/nntp.go:238: // Internal. Gets the next NNTP response.
I suggest refactoring the helpers a bit.  

// cmd executes the beginning of an NNTP command:
// it sends the command given by the format and arguments
// and then reads the response line.  If expectCode > 0, the
// status code on the response line must match it.
// 1-digit expectCodes only check the first digit of the status code,
// and so on.
func (c *Conn) cmd(expectCode int, format string, args ...interface{}) (code
int, line string, err os.Error) {
    if c.body != nil {
        c.body.discard()
        c.body = nil
    }
    if err := fmt.Fprintf(c.conn, format+"\r\n", args); err != nil {
        return 0, "", err
    }
    line, err = c.r.ReadString('\n')
    if err != nil {
        return 0, "", err
    }
    line = strings.TrimSpace(line)
    if len(line) < 4 || line[3] != ' ' {
        return 0, "", ProtocolError("short response: " + line)
    }
    code, err := strconv.Atoui(line[0:3])
    if err != nil {
        return ProtocolError("invalid response code: " + line)
    }
    line = line[4:]
    if 1 <= expectCode && expectCode < 10 && code/100 != expectCode ||
        10 <= expectCode && expectCode < 100 && code/10 != expectCode ||
        100 <= expectCode && expectCode < 1000 && code != expectCode {
        err = Error{code, line}
    }
    return
}

// bodyReader satisfies reads by reading from the connection
// until it finds a line containing just .
type bodyReader struct {
    c *Conn
    eof bool
}

func (r *bodyReader) Read(b []byte) (n int, err os.Error) {
    // read until . on line by itself.
    // s/^.././
    // set r.eof = true when . is found
}

func (r *bodyReader) discard() {
    // read until ., discarding the data
    // (if r.eof is not yet true)
}

func (c *Conn) body() (io.Reader, os.Error) {
    c.body = &bodyReader{c: c}
    return c.body, nil
}

// readStrings reads a list of strings from the NNTP connection,
// stopping at a line containing only a .
// convenience method for LIST, etc.
func (c *Conn) readStrings() ([]string, os.Error) {
    // 
...
}

http://codereview.appspot.com/808041/diff/2001/3003#newcode336
src/pkg/nntp/nntp.go:336: func timeStrings(t *time.Time) (string, string) {
Why return two strings?

return fmt.Sprintf("%04d%02d%02d %02d%02d%02d",
    t.Year, t.Month, t.Day, t.Hour, t.Minute, t.Second)

http://codereview.appspot.com/808041/diff/2001/3003#newcode343
src/pkg/nntp/nntp.go:343: func (n *Conn) ModeReader() os.Error {
Since the client is not a transferrer anyway, should this
be done always?  Also, is it even necessary?
This is the sort of detail that it seems like the library
should hide from the client.  Maybe the login + auth +
capabilities + MODE READER (if needed) should all
be done as part of Dial?  Capabilities could be stored
in a map[string]bool in the Conn.

http://codereview.appspot.com/808041/diff/2001/3003#newcode344
src/pkg/nntp/nntp.go:344: resp, err := n.shortCmd("MODE READER")
_, _, err := c.cmd(20, "MODE READER")
return err

Do you want to record whether posting is allowed?

http://codereview.appspot.com/808041/diff/2001/3003#newcode356
src/pkg/nntp/nntp.go:356: datestr, timestr := timeStrings(since)
if _, _, err := c.cmd(231, "NEWGROUPS %s GMT", timefmt(since)); err != nil {
    return nil, err
}
return c.readGroups()

http://codereview.appspot.com/808041/diff/2001/3003#newcode371
src/pkg/nntp/nntp.go:371: datestr, timestr := timeStrings(since)
can use c.cmd 230 + c.readStrings.

then sort + uniq the articles:

sort.SortStrings(id)
w := 0
for r, s := range id {
    if r == 0 || id[r-1] != s {
        id[w] = s
        w++
    }
}
id = id[0:w]

http://codereview.appspot.com/808041/diff/2001/3003#newcode405
src/pkg/nntp/nntp.go:405: func parseGroupStates(msg string) []*GroupStatus {
func (c *Conn) readGroups() ([]Group, os.Error) {
   readStrings()
   then parse
}


groups are small enough that it's probably worthwhile
making it a []Group instead of []*Group.

http://codereview.appspot.com/808041/diff/2001/3003#newcode448
src/pkg/nntp/nntp.go:448: resp, err := n.longCmd("CAPABILITIES", nil)
usual c.cmd 101 + c.readStrings

http://codereview.appspot.com/808041/diff/2001/3003#newcode462
src/pkg/nntp/nntp.go:462: resp, err := n.shortCmd("DATE")
usual c.cmd 111, but i'd save the line and
try to parse it with time.Parse:

// at top of file
const timeFormat = "20060102 150405"

// Date()
_, line, err := c.cmd(111, "DATE")
if err != nil {
    return nil, err
}
t, err := time.Parse(timeFormat, line)
if err != nil {
    return nil, ProtocolError("invalid time: " + line)
}
return t, nil

We'll need to make time.Parse and time.Format work with those run-together time
formats, but that should be an easy change.
Then the formatting function above can be since.Format(timeFormat)
instead of the longer fmt.Sprintf call.

http://codereview.appspot.com/808041/diff/2001/3003#newcode498
src/pkg/nntp/nntp.go:498: resp, err := n.longCmd("LIST", nil)
c.cmd 215 + c.readGroups.

http://codereview.appspot.com/808041/diff/2001/3003#newcode512
src/pkg/nntp/nntp.go:512: func (n *Conn) List(keyword, wildmat string)
([]string, os.Error) {
Arguments look ignored.
c.cmd 215 + c.readStrings

http://codereview.appspot.com/808041/diff/2001/3003#newcode527
src/pkg/nntp/nntp.go:527: resp, err := n.shortCmd("GROUP " + group)
_, line, err := c.cmd(211, "GROUP %s", group)
...

http://codereview.appspot.com/808041/diff/2001/3003#newcode529
src/pkg/nntp/nntp.go:529: number, low, high = -1, -1, -1
Convention is to leave things with the zero value when err != nil.

http://codereview.appspot.com/808041/diff/2001/3003#newcode540
src/pkg/nntp/nntp.go:540: numbers := [3]int{-1, -1, -1}
var i [3]int
would do just as well

http://codereview.appspot.com/808041/diff/2001/3003#newcode543
src/pkg/nntp/nntp.go:543: if len(ss) <= i {
why do you allow != 3 fields?

http://codereview.appspot.com/808041/diff/2001/3003#newcode552
src/pkg/nntp/nntp.go:552: err = nil
err is guaranteed nil already

http://codereview.appspot.com/808041/diff/2001/3003#newcode557
src/pkg/nntp/nntp.go:557: func (n *Conn) Help() (string, os.Error) {
func (c *Conn) Help() (io.Reader, os.Error) {
    if _, _, err := n.cmd(100, "HELP"); err != nil {
        return nil, err
    }
    c.body = &bodyReader{c:c}
    return c.body, nil
}

http://codereview.appspot.com/808041/diff/2001/3003#newcode571
src/pkg/nntp/nntp.go:571: func parseHeaders(headers string) (*Header, os.Error)
{
Like the one in http, this should be reading from
the connection instead of from a string.

func (c *Conn) readHeader() (*Article, os.Error)

http://codereview.appspot.com/808041/diff/2001/3003#newcode670
src/pkg/nntp/nntp.go:670: resp, err := n.shortCmd(maybeId(cmd, id))
c.cmd 213

http://codereview.appspot.com/808041/diff/2001/3003#newcode703
src/pkg/nntp/nntp.go:703: func (n *Conn) ArticleFile(id string, w io.Writer)
os.Error {
It's a bit awkward to use an io.Writer here.
Typically io.Writers are for things that leave the program,
but this is probably something trying to come into the program: the caller wants
to read the article.

func (c *Conn) RawArticle(id string) (io.Reader, os.Error)
func (c *Conn) Article(id string) (*Article, os.Error)

http://codereview.appspot.com/808041/diff/2001/3003#newcode745
src/pkg/nntp/nntp.go:745: // Performs an NNTP 'HEAD' command, parsing the
headers.
probably return *Article with a nil Body

http://codereview.appspot.com/808041/diff/2001/3003#newcode764
src/pkg/nntp/nntp.go:764: func (n *Conn) Body(id string) (string, os.Error) {
func (c *Conn) Body(id string) (io.Reader, os.Error)

http://codereview.appspot.com/808041/diff/2001/3003#newcode794
src/pkg/nntp/nntp.go:794: func (n *Conn) PostFile(r *bufio.Reader) os.Error {
func (c *Conn) RawPost(r io.Reader) os.Error
func (c *Conn) Post(a *Article) os.Error

http://codereview.appspot.com/808041/diff/2001/3003#newcode847
src/pkg/nntp/nntp.go:847: func (n *Conn) Quit() os.Error {
Should probably record that the connection is closed.

http://codereview.appspot.com/808041/diff/2001/3004
File src/pkg/nntp/nntp_test.go (right):

http://codereview.appspot.com/808041/diff/2001/3004#newcode20
src/pkg/nntp/nntp_test.go:20: conn, err := Connect("news.gmane.org:119", "", "")
I'd like not to introduce any more tests that depend on
a working network connection.  (The ones in package net
and package http need to go away.)

Perhaps an alternate testing strategy would be to write
an expected transcript for a sequence of commands
and then run those commands, collecting results.
Something like:

var response = `200 OK hi how are you
...
`

var request = `USER whatever...
...
`)

var outbuf bytes.Buffer
var fake struct {
    io.Reader
    io.Writer
}
fake.Reader = strings.NewReader(response)
fake.Writer = &outbuf
c := dial(&fake)

(assuming dial is what Dial calls to do everything after
establishing the TCP connection).

then run the sequence of commands here, checking that
the nntp client handles the responses correctly.
and then at the end, check that outbuf.String == request.

[cemeyer, 2010-03-29 04:22:59]

I think I've addressed all of the issues raised in the first round of reviews,
and I'll mail the new changeset in a second.

http://codereview.appspot.com/808041/diff/2001/3002
File src/pkg/nntp/Makefile (right):

http://codereview.appspot.com/808041/diff/2001/3002#newcode5
src/pkg/nntp/Makefile:5: include ../../../src/Make.$(GOARCH)
On 2010/03/28 07:36:29, rsc wrote:
> include ../../Make.$(GOARCH)
> 

Are you sure? It built with ../../.., with ../.. it breaks make.

http://codereview.appspot.com/808041/diff/2001/3002#newcode11
src/pkg/nntp/Makefile:11: include ../../../src/Make.pkg
On 2010/03/28 07:36:29, rsc wrote:
> include ../../Make.pkg
> 

(Same.)

http://codereview.appspot.com/808041/diff/2001/3003
File src/pkg/nntp/nntp.go (right):

http://codereview.appspot.com/808041/diff/2001/3003#newcode5
src/pkg/nntp/nntp.go:5: // This package provides a programmatic interface for
NNTP clients.
On 2010/03/28 07:36:29, rsc wrote:
> "programmatic" is redundant in most programs.
> 
> // The nntp package implements a client for the news protocol NNTP,
> // as defined in RFC 3977.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode25
src/pkg/nntp/nntp.go:25: // Represents an error message from the NNTP server.
On 2010/03/28 07:36:29, rsc wrote:
> Doc comments should be full sentences.
> See http://golang.org/doc/effective_go.html#commentary
> 

Changed to 'A struct to represent error responses from the NNTP server.'

http://codereview.appspot.com/808041/diff/2001/3003#newcode26
src/pkg/nntp/nntp.go:26: type Error struct {
On 2010/03/28 07:36:29, rsc wrote:
> This error seems to serve multiple purposes:
> it represents more than just an error message 
> from the NNTP server.  Maybe it should go back
> to being just an NNTP server error and the other
> cases should be some other kind of error, like
> 
> type ProtocolError string
> 

It would probably be most accurate to say it's the numeric code and rest of the
response line from the server, less any sort of message, if the response had a
message; however, it's only returned when that response confuses the library
somehow. This could happen if, for example, the user calls Conn.Capabilities()
on a server that doesn't support it -- an example server responds with "500
What?".

By multiple purposes, are you mostly referring to it being used for both
'newError()' (to encapsulate server responses) and 'newErrorStr()' (which is
wholly unrelated)? If so, this makes a lot of sense to me.

http://codereview.appspot.com/808041/diff/2001/3003#newcode33
src/pkg/nntp/nntp.go:33: realsock   net.Conn
On 2010/03/28 07:36:29, rsc wrote:
> Please avoid the parochial term socket.
> The rest of the network libraries do not use it.
> 
> conn net.Conn
> r *bufio.Reader
> 
> would be fine.

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode35
src/pkg/nntp/nntp.go:35: user, pass string
On 2010/03/28 07:36:29, rsc wrote:
> Can avoid storing these in the struct.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode36
src/pkg/nntp/nntp.go:36: noposting  bool
On 2010/03/28 07:36:29, rsc wrote:
> Avoid double negative: canPost bool
> 

I'm not even sure anymore that it's a useful distinction -- it's based on the
server greeting, and may change after the user authenticates, for example. I
think I'll nuke it for now.

http://codereview.appspot.com/808041/diff/2001/3003#newcode40
src/pkg/nntp/nntp.go:40: type nntpResponse struct {
On 2010/03/28 07:36:29, rsc wrote:
> The nntp here is redundant.  type response struct {
> But maybe the struct isn't needed either.  (See below.)
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode46
src/pkg/nntp/nntp.go:46: // The status of a group named 'Name'. High and Low
indicate the highest and lowest
On 2010/03/28 07:36:29, rsc wrote:
> Can drop quotes around Name.
> Quotes on next line should be "" not '' (they are strings).
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode49
src/pkg/nntp/nntp.go:49: type GroupStatus struct {
On 2010/03/28 07:36:29, rsc wrote:
> There doesn't appear to be a Group, so GroupStatus seems
> unnecessarily verbose.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode57
src/pkg/nntp/nntp.go:57: From    string
On 2010/03/28 07:36:29, rsc wrote:
> Not sure why these are pulled out of the header map.
> 

Nuked.

http://codereview.appspot.com/808041/diff/2001/3003#newcode60
src/pkg/nntp/nntp.go:60: Headers map[string][]string
On 2010/03/28 07:36:29, rsc wrote:
> More common (for example, in http.Request) to be singular:
> Group []string, Header map[string][]string
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode65
src/pkg/nntp/nntp.go:65: *Header
On 2010/03/28 07:36:29, rsc wrote:
> You can embed a Header directly without the *.
> That's probably better here.
> 
> Or maybe these should be one type, Article,
> and a header-only article has Body == nil.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode66
src/pkg/nntp/nntp.go:66: Body string
On 2010/03/28 07:36:29, rsc wrote:
> Large data block such as article bodies probably
> make more sense as []byte than as string.
> Even better for large bodies, it might make
> sense for Body to be an io.Reader, and require
> the caller to read what it wants of the article
> before it issues the next command.
> I've continued to sketch out this latter approach below.
> 

Switched to []byte for now.

http://codereview.appspot.com/808041/diff/2001/3003#newcode69
src/pkg/nntp/nntp.go:69: func (n *Header) String() string {
On 2010/03/28 07:36:29, rsc wrote:
> String should return something short, usually on
> a single line.  For example, String() might be
> [NNTP article <message-id>].
> 
> The functionality here  should be something like
> 
> func (h *Header) WriteTo(w io.Writer) os.Error 
> 
> which won't need to construct a giant string.
> Also, WriteTo should use the standard Go newline
> convention \n instead of \r\n.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode80
src/pkg/nntp/nntp.go:80: func (n *Article) String() string {
On 2010/03/28 07:36:29, rsc wrote:
> Same comment about String.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode89
src/pkg/nntp/nntp.go:89: if n.Code == 1000 {
On 2010/03/28 07:36:29, rsc wrote:
> This should be a different kind of error.
> Otherwise clients have to know that sometimes
> n.Code == 1000 to mean something completely 
> different.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode96
src/pkg/nntp/nntp.go:96: typeStr = "Informative message"
On 2010/03/28 07:36:29, rsc wrote:
> I don't think these strings add much, and they make the function 40 lines
long. 
> I suggest instead:
> 
> return fmt.Sprintf("%d %s", n.Code, n.Msg)
> 

True enough. They were somewhat helpful to me debugging, but anyone else can go
look up RFC 3977 too. Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode156
src/pkg/nntp/nntp.go:156: func Connect(host, username, password string) (*Conn,
os.Error) {
On 2010/03/28 07:36:29, rsc wrote:
> In the Go libraries this is typically called Dial, not Connect.
> 

Done. My original reasoning was that the semantic action isn't exactly the same
as that of e.g. net.Dial() -- we (potentially) authenticate as well. But I'm
happy using Dial() instead if you think it fits.

http://codereview.appspot.com/808041/diff/2001/3003#newcode187
src/pkg/nntp/nntp.go:187: func (n *Conn) Authenticate() os.Error {
On 2010/03/28 07:36:29, rsc wrote:
> Does this need to be public?  It looks like no.
> Alternately, maybe it should stay public, take over
> the username and password arguments from Dial,
> and not be called by default - not all NNTP servers
> need a user name and password.
> 

I took the second approach -- keep it public, don't call it in Dial().

http://codereview.appspot.com/808041/diff/2001/3003#newcode187
src/pkg/nntp/nntp.go:187: func (n *Conn) Authenticate() os.Error {
On 2010/03/28 07:36:29, rsc wrote:
> n is an odd name for a *Conn.  c?
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode209
src/pkg/nntp/nntp.go:209: line = line + "\r\n"
On 2010/03/28 07:36:29, rsc wrote:
> return fmt.Fprintf(n.conn, "%s\r\n", line)
> 
> Or, better just call that directly where this function is used.
> 

Removed putLine(), replaced with direct calls to fmt.Fprintf().

http://codereview.appspot.com/808041/diff/2001/3003#newcode230
src/pkg/nntp/nntp.go:230: res, err := n.sock.ReadString('\n')
On 2010/03/28 07:36:29, rsc wrote:
> return c.r.ReadString('\n')
> 
> Or, better, just call that directly where this function is used.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode238
src/pkg/nntp/nntp.go:238: // Internal. Gets the next NNTP response.
On 2010/03/28 07:36:29, rsc wrote:
> (snipped)
> 
> func (c *Conn) body() (io.Reader, os.Error) {
>     c.body = &bodyReader{c: c}
>     return c.body, nil
> }

Why does body return os.Error? In places where this is useful (e.g. Conn.Help()
-> (io.Reader, os.Error)), one can use:
  return c.body(), nil
instead of (what I'm guessing you had in mind):
  return c.body()

Thoughts?

http://codereview.appspot.com/808041/diff/2001/3003#newcode336
src/pkg/nntp/nntp.go:336: func timeStrings(t *time.Time) (string, string) {
On 2010/03/28 07:36:29, rsc wrote:
> Why return two strings?
> 
> return fmt.Sprintf("%04d%02d%02d %02d%02d%02d",
>     t.Year, t.Month, t.Day, t.Hour, t.Minute, t.Second)
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode343
src/pkg/nntp/nntp.go:343: func (n *Conn) ModeReader() os.Error {
On 2010/03/28 07:36:29, rsc wrote:
> Since the client is not a transferrer anyway, should this
> be done always?  Also, is it even necessary?
> This is the sort of detail that it seems like the library
> should hide from the client.  Maybe the login + auth +
> capabilities + MODE READER (if needed) should all
> be done as part of Dial?  Capabilities could be stored
> in a map[string]bool in the Conn.

Many (currently deployed) servers do not support CAPABILITIES, and MODE READER
is only required for "mode-switching servers", which is some subset (I'd guess a
proportionately small subset) of existing NNTP servers.

Capabilities can change after authentication (or even vary arbitrarily through
the NNTP session) and shouldn't be cached at the beginning.

Re: is it neccessary? I think library users will know best here (if their
particular server does or does not need MODE READER).

http://codereview.appspot.com/808041/diff/2001/3003#newcode344
src/pkg/nntp/nntp.go:344: resp, err := n.shortCmd("MODE READER")
On 2010/03/28 07:36:29, rsc wrote:
> _, _, err := c.cmd(20, "MODE READER")
> return err

Done.
 
> Do you want to record whether posting is allowed?

Nope (nuked that field from the Conn struct). Even the RFC points out that the
initial greeting / responses to MODE READER are unreliable. The most reliable
approach seems to be to send a POST command and check for a 3xx response.

http://codereview.appspot.com/808041/diff/2001/3003#newcode356
src/pkg/nntp/nntp.go:356: datestr, timestr := timeStrings(since)
On 2010/03/28 07:36:29, rsc wrote:
> if _, _, err := c.cmd(231, "NEWGROUPS %s GMT", timefmt(since)); err != nil {
>     return nil, err
> }
> return c.readGroups()
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode371
src/pkg/nntp/nntp.go:371: datestr, timestr := timeStrings(since)
On 2010/03/28 07:36:29, rsc wrote:
> can use c.cmd 230 + c.readStrings.
> 
> then sort + uniq the articles:
> 
> sort.SortStrings(id)
> w := 0
> for r, s := range id {
>     if r == 0 || id[r-1] != s {
>         id[w] = s
>         w++
>     }
> }
> id = id[0:w]
> 
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode405
src/pkg/nntp/nntp.go:405: func parseGroupStates(msg string) []*GroupStatus {
On 2010/03/28 07:36:29, rsc wrote:
> func (c *Conn) readGroups() ([]Group, os.Error) {
>    readStrings()
>    then parse
> }
> 
> 
> groups are small enough that it's probably worthwhile
> making it a []Group instead of []*Group.

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode448
src/pkg/nntp/nntp.go:448: resp, err := n.longCmd("CAPABILITIES", nil)
On 2010/03/28 07:36:29, rsc wrote:
> usual c.cmd 101 + c.readStrings
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode462
src/pkg/nntp/nntp.go:462: resp, err := n.shortCmd("DATE")
On 2010/03/28 07:36:29, rsc wrote:
> usual c.cmd 111, but i'd save the line and
> try to parse it with time.Parse:
> 
> // at top of file
> const timeFormat = "20060102 150405"
> 
> // Date()
> _, line, err := c.cmd(111, "DATE")
> if err != nil {
>     return nil, err
> }
> t, err := time.Parse(timeFormat, line)
> if err != nil {
>     return nil, ProtocolError("invalid time: " + line)
> }
> return t, nil
> 
> We'll need to make time.Parse and time.Format work with those run-together
time
> formats, but that should be an easy change.
> Then the formatting function above can be since.Format(timeFormat)
> instead of the longer fmt.Sprintf call.

Done (though from the existing Format / Parse code in time, it looks like adding
these formats will be a huge hack).

Something to keep in mind: the format returned by the server in response to DATE
is actually different from that sent by the client for NEWNEWS and NEWGROUPS.

http://codereview.appspot.com/808041/diff/2001/3003#newcode498
src/pkg/nntp/nntp.go:498: resp, err := n.longCmd("LIST", nil)
On 2010/03/28 07:36:29, rsc wrote:
> c.cmd 215 + c.readGroups.
> 
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode512
src/pkg/nntp/nntp.go:512: func (n *Conn) List(keyword, wildmat string)
([]string, os.Error) {
On 2010/03/28 07:36:29, rsc wrote:
> Arguments look ignored.

Oops. Fixed.

> c.cmd 215 + c.readStrings
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode527
src/pkg/nntp/nntp.go:527: resp, err := n.shortCmd("GROUP " + group)
On 2010/03/28 07:36:29, rsc wrote:
> _, line, err := c.cmd(211, "GROUP %s", group)
> ...
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode529
src/pkg/nntp/nntp.go:529: number, low, high = -1, -1, -1
On 2010/03/28 07:36:29, rsc wrote:
> Convention is to leave things with the zero value when err != nil.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode543
src/pkg/nntp/nntp.go:543: if len(ss) <= i {
On 2010/03/28 07:36:29, rsc wrote:
> why do you allow != 3 fields?
> 

Fixed.

http://codereview.appspot.com/808041/diff/2001/3003#newcode552
src/pkg/nntp/nntp.go:552: err = nil
On 2010/03/28 07:36:29, rsc wrote:
> err is guaranteed nil already
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode557
src/pkg/nntp/nntp.go:557: func (n *Conn) Help() (string, os.Error) {
On 2010/03/28 07:36:29, rsc wrote:
> func (c *Conn) Help() (io.Reader, os.Error) {
>     if _, _, err := n.cmd(100, "HELP"); err != nil {
>         return nil, err
>     }
>     c.body = &bodyReader{c:c}
>     return c.body, nil
> }
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode571
src/pkg/nntp/nntp.go:571: func parseHeaders(headers string) (*Header, os.Error)
{
On 2010/03/28 07:36:29, rsc wrote:
> Like the one in http, this should be reading from
> the connection instead of from a string.
> 
> func (c *Conn) readHeader() (*Article, os.Error)
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode670
src/pkg/nntp/nntp.go:670: resp, err := n.shortCmd(maybeId(cmd, id))
On 2010/03/28 07:36:29, rsc wrote:
> c.cmd 213
> 

223, but Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode703
src/pkg/nntp/nntp.go:703: func (n *Conn) ArticleFile(id string, w io.Writer)
os.Error {
On 2010/03/28 07:36:29, rsc wrote:
> It's a bit awkward to use an io.Writer here.
> Typically io.Writers are for things that leave the program,
> but this is probably something trying to come into the program: the caller
wants
> to read the article.
> 
> func (c *Conn) RawArticle(id string) (io.Reader, os.Error)
> func (c *Conn) Article(id string) (*Article, os.Error)
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode745
src/pkg/nntp/nntp.go:745: // Performs an NNTP 'HEAD' command, parsing the
headers.
On 2010/03/28 07:36:29, rsc wrote:
> probably return *Article with a nil Body
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode764
src/pkg/nntp/nntp.go:764: func (n *Conn) Body(id string) (string, os.Error) {
On 2010/03/28 07:36:29, rsc wrote:
> func (c *Conn) Body(id string) (io.Reader, os.Error)
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode794
src/pkg/nntp/nntp.go:794: func (n *Conn) PostFile(r *bufio.Reader) os.Error {
On 2010/03/28 07:36:29, rsc wrote:
> func (c *Conn) RawPost(r io.Reader) os.Error
> func (c *Conn) Post(a *Article) os.Error
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3003#newcode847
src/pkg/nntp/nntp.go:847: func (n *Conn) Quit() os.Error {
On 2010/03/28 07:36:29, rsc wrote:
> Should probably record that the connection is closed.
> 

Done.

http://codereview.appspot.com/808041/diff/2001/3004
File src/pkg/nntp/nntp_test.go (right):

http://codereview.appspot.com/808041/diff/2001/3004#newcode20
src/pkg/nntp/nntp_test.go:20: conn, err := Connect("news.gmane.org:119", "", "")
On 2010/03/28 07:36:29, rsc wrote:
> I'd like not to introduce any more tests that depend on
> a working network connection.  (The ones in package net
> and package http need to go away.)
> 
> Perhaps an alternate testing strategy would be to write
> an expected transcript for a sequence of commands
> and then run those commands, collecting results.
> Something like:
> 
> var response = `200 OK hi how are you
> ...
> `
> 
> var request = `USER whatever...
> ...
> `)
> 
> var outbuf bytes.Buffer
> var fake struct {
>     io.Reader
>     io.Writer
> }
> fake.Reader = strings.NewReader(response)
> fake.Writer = &outbuf
> c := dial(&fake)
> 
> (assuming dial is what Dial calls to do everything after
> establishing the TCP connection).
> 
> then run the sequence of commands here, checking that
> the nntp client handles the responses correctly.
> and then at the end, check that outbuf.String == request.

Done.

[cemeyer, 2010-03-29 04:23:52]

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

Please take another look.

[rsc, 2010-03-29 05:22:28]

Looks much better.  A few things remain.
In addition to the code notes below, the comments
still need work.  Comments should be full sentences
that start with the name of the thing being described.
This is different from Java and other languages and admits
a greater variety of presentations.  See
http://golang.org/doc/effective_go.html#commentary for more.

Examples:

// An Error represents an error response from an NNTP server.

// NewNews returns a list of the IDs of articles posted 
// since the given time.

...

http://codereview.appspot.com/808041/diff/2001/3002
File src/pkg/nntp/Makefile (right):

http://codereview.appspot.com/808041/diff/2001/3002#newcode5
src/pkg/nntp/Makefile:5: include ../../../src/Make.$(GOARCH)
On 2010/03/29 04:22:59, cemeyer wrote:
> On 2010/03/28 07:36:29, rsc wrote:
> > include ../../Make.$(GOARCH)
> > 
> 
> Are you sure? It built with ../../.., with ../.. it breaks make.

I believe it breaks with

../../src/Make.$(GOARCH)

but it should work fine with

../../Make.$(GOARCH)

http://codereview.appspot.com/808041/diff/2001/3003
File src/pkg/nntp/nntp.go (right):

http://codereview.appspot.com/808041/diff/2001/3003#newcode238
src/pkg/nntp/nntp.go:238: // Internal. Gets the next NNTP response.
On 2010/03/29 04:22:59, cemeyer wrote:
> On 2010/03/28 07:36:29, rsc wrote:
> > (snipped)
> > 
> > func (c *Conn) body() (io.Reader, os.Error) {
> >     c.body = &bodyReader{c: c}
> >     return c.body, nil
> > }
> 
> Why does body return os.Error? In places where this is useful (e.g.
Conn.Help()
> -> (io.Reader, os.Error)), one can use:
>   return c.body(), nil
> instead of (what I'm guessing you had in mind):
>   return c.body()
> 
> Thoughts?

You're right: c.body() returning io.Reader makes more sense.

http://codereview.appspot.com/808041/diff/2001/3003#newcode343
src/pkg/nntp/nntp.go:343: func (n *Conn) ModeReader() os.Error {
> Re: is it neccessary? I think library users will know best here (if their
> particular server does or does not need MODE READER).

This is the library author's fallacy: if you don't know best
how can the client of the library, who knows less about NNTP
almost by definition, decide?  I certainly don't know, as a
prospective client of this package, what I should be doing.
It sounds like the right thing is subtle, but that's all the more
reason to get it right once, in the library.

Also, the RFC says that CAPABILITIES is mandatory.
Are you sure that many currently deployed servers
do not support it?

http://codereview.appspot.com/808041/diff/11001/12003#newcode26
src/pkg/nntp/nntp.go:26: type Error struct {
do you have another CL to make this work?
when i tried it last night it looked like package time
needed a little tweaking.

http://codereview.appspot.com/808041/diff/11001/12003#newcode29
src/pkg/nntp/nntp.go:29: }
now unused

http://codereview.appspot.com/808041/diff/11001/12003#newcode89
src/pkg/nntp/nntp.go:89: if n.Code == 1000 {
you could use []byte(".\n") too.
either way it's an allocation.  better to put it at top level

var dotnl = []byte(".\n")
var dotdot = []byte("..")

http://codereview.appspot.com/808041/diff/11001/12003#newcode117
src/pkg/nntp/nntp.go:117: categoryStr = "Distribution functions"
The logic here seems a bit odd.  I would have 
expected a more chronological sequence.

if r.headerbuf == nil {
    ...
}
if !r.headerdone {
    n, err = r.headerbuf.Read(p)
    if err == os.EOF {
        err = nil
        r.headerdone = true
    }
    if n > 0 {
        return
    }
}
if r.a.Body != nil {
    n, err = r.a.Body.Read(p)
    if err == os.EOF {
        r.a.Body = nil
    }
    return
}
return 0, os.EOF

http://codereview.appspot.com/808041/diff/11001/12003#newcode128
src/pkg/nntp/nntp.go:128: return fmt.Sprintf("%03d: (%s; %s): %s", n.Code,
typeStr, categoryStr, n.Msg)
buf := new(bytes.Buffer)
and then you can fmt.Fprintf into it to build the header.

http://codereview.appspot.com/808041/diff/11001/12003#newcode137
src/pkg/nntp/nntp.go:137: }
Only if Body != nil?

http://codereview.appspot.com/808041/diff/11001/12003#newcode143
src/pkg/nntp/nntp.go:143: err.Code = 1000
Have you tested this?
Seems like it will return os.EOF early.

(The line Rietveld is quoting (about Code = 1000)
is stale.  This is about the err == os.EOF check.
It displays right on the main page.)

http://codereview.appspot.com/808041/diff/11001/12003#newcode233
src/pkg/nntp/nntp.go:233: }
Looks like the logic here can be shortened to

code, _, err := c.cmd(2, "AUTHINFO USER %s", username)
if code/100 == 3 {
    _, _, err = c.cmd(2, "AUTHINFO PASS %s", password)
}
return err

http://codereview.appspot.com/808041/diff/11001/12003#newcode302
src/pkg/nntp/nntp.go:302: }
lines?

http://codereview.appspot.com/808041/diff/11001/12003#newcode442
src/pkg/nntp/nntp.go:442: return realres
Perhaps move these to the bottom of the file
under a header

// Everything below this line is copied from package http.
// It should be factored out into a common library.

http://codereview.appspot.com/808041/diff/11001/12003#newcode597
src/pkg/nntp/nntp.go:597: goto Malformed
These all have specific codes that should come back
(220 in this example).  Seems like it would be good
practice to check for the specific code.

http://codereview.appspot.com/808041/diff/11001/12003#newcode608
src/pkg/nntp/nntp.go:608: // Look for extension lines
a, err

Also, have you tested this? 
c.readHeader calls c.body once, reading from it
with a bufio.Reader, and then res.Body below
calls c.body again.  Since the bufio.Reader is allowed
to read beyond what is requested, the second c.body
is probably missing some of the data.

http://codereview.appspot.com/808041/diff/11001/12003#newcode663
src/pkg/nntp/nntp.go:663: 
prepend really means to ponder deeply.

easy to avoid: the string is a prefix anyway

http://codereview.appspot.com/808041/diff/11001/12004
File src/pkg/nntp/nntp_test.go (right):

http://codereview.appspot.com/808041/diff/11001/12004#newcode34
src/pkg/nntp/nntp_test.go:34: var lines = `101 Capability list:
move this and other big string out to top level
(and maybe the bottom of the file) just for ease of readability.

var basicServer =
var basicClient =

[cemeyer, 2010-03-29 19:59:01]

Ok, I believe I've addressed the code complaints this time around. I've added a
couple things to the testing, and made a pass over nntp.go to clean up the doc
comments.

http://codereview.appspot.com/808041/diff/2001/3002
File src/pkg/nntp/Makefile (right):

http://codereview.appspot.com/808041/diff/2001/3002#newcode5
src/pkg/nntp/Makefile:5: include ../../../src/Make.$(GOARCH)
On 2010/03/29 05:22:29, rsc wrote:
> On 2010/03/29 04:22:59, cemeyer wrote:
> > On 2010/03/28 07:36:29, rsc wrote:
> > > include ../../Make.$(GOARCH)
> > > 
> > 
> > Are you sure? It built with ../../.., with ../.. it breaks make.
> 
> I believe it breaks with
> 
> ../../src/Make.$(GOARCH)
> 
> but it should work fine with
> 
> ../../Make.$(GOARCH)

My mistake. Done.

http://codereview.appspot.com/808041/diff/2001/3003
File src/pkg/nntp/nntp.go (right):

http://codereview.appspot.com/808041/diff/2001/3003#newcode343
src/pkg/nntp/nntp.go:343: func (n *Conn) ModeReader() os.Error {
On 2010/03/29 05:22:29, rsc wrote:
> > Re: is it neccessary? I think library users will know best here (if their
> > particular server does or does not need MODE READER).
> 
> This is the library author's fallacy: if you don't know best
> how can the client of the library, who knows less about NNTP
> almost by definition, decide?  I certainly don't know, as a
> prospective client of this package, what I should be doing.
> It sounds like the right thing is subtle, but that's all the more
> reason to get it right once, in the library.
> 
> Also, the RFC says that CAPABILITIES is mandatory.
> Are you sure that many currently deployed servers
> do not support it?

I guess not all NNTP servers follow the latest RFC (3977 was published in 2006,
if I remember correctly). I know of at least one NNTP server with a large number
of users that doesn't support it (it was the first one I tried -- I didn't go
hunting for one without CAPABILITIES).

http://codereview.appspot.com/808041/diff/11001/12003#newcode26
src/pkg/nntp/nntp.go:26: type Error struct {
On 2010/03/29 05:22:29, rsc wrote:
> do you have another CL to make this work?
> when i tried it last night it looked like package time
> needed a little tweaking.

(Sorry, what's a CL?) I haven't added this functionality to the time package
functions (Parse / Format) yet because it looks like they expect some sort of
separation between the fields of a date/time string -- adding these two formats
will be a big hack.

http://codereview.appspot.com/808041/diff/11001/12003#newcode29
src/pkg/nntp/nntp.go:29: }
On 2010/03/29 05:22:29, rsc wrote:
> now unused
> 

Done.

http://codereview.appspot.com/808041/diff/11001/12003#newcode89
src/pkg/nntp/nntp.go:89: if n.Code == 1000 {
On 2010/03/29 05:22:29, rsc wrote:
> you could use []byte(".\n") too.
> either way it's an allocation.  better to put it at top level
> 
> var dotnl = []byte(".\n")
> var dotdot = []byte("..")
> 

Done.

http://codereview.appspot.com/808041/diff/11001/12003#newcode117
src/pkg/nntp/nntp.go:117: categoryStr = "Distribution functions"
On 2010/03/29 05:22:29, rsc wrote:
> The logic here seems a bit odd.  I would have 
> expected a more chronological sequence.
> 
> if r.headerbuf == nil {
>     ...
> }
> if !r.headerdone {
>     n, err = r.headerbuf.Read(p)
>     if err == os.EOF {
>         err = nil
>         r.headerdone = true
>     }
>     if n > 0 {
>         return
>     }
> }
> if r.a.Body != nil {
>     n, err = r.a.Body.Read(p)
>     if err == os.EOF {
>         r.a.Body = nil
>     }
>     return
> }
> return 0, os.EOF
> 

Done.

http://codereview.appspot.com/808041/diff/11001/12003#newcode128
src/pkg/nntp/nntp.go:128: return fmt.Sprintf("%03d: (%s; %s): %s", n.Code,
typeStr, categoryStr, n.Msg)
On 2010/03/29 05:22:29, rsc wrote:
> buf := new(bytes.Buffer)
> and then you can fmt.Fprintf into it to build the header.
> 

Done.

http://codereview.appspot.com/808041/diff/11001/12003#newcode137
src/pkg/nntp/nntp.go:137: }
On 2010/03/29 05:22:29, rsc wrote:
> Only if Body != nil?

Body == nil represents the situation where the Article object isn't really a
full article, but is just the Header -- for NNTP, using the HEAD command, you
get output like:
> Foo: bar
> Bar: baz
> .
That is, there is no trailing empty line like there is with a full ARTICLE. I
think it makes more sense without the newline if there is no body, but I don't
feel strongly about it.

http://codereview.appspot.com/808041/diff/11001/12003#newcode143
src/pkg/nntp/nntp.go:143: err.Code = 1000
On 2010/03/29 05:22:29, rsc wrote:
> Have you tested this?
> Seems like it will return os.EOF early.

The code catches os.EOF and replaces it with nil. It only returns if it has read
something. I added a test for it.

http://codereview.appspot.com/808041/diff/11001/12003#newcode233
src/pkg/nntp/nntp.go:233: }
On 2010/03/29 05:22:29, rsc wrote:
> Looks like the logic here can be shortened to
> 
> code, _, err := c.cmd(2, "AUTHINFO USER %s", username)
> if code/100 == 3 {
>     _, _, err = c.cmd(2, "AUTHINFO PASS %s", password)
> }
> return err
> 

Done.

http://codereview.appspot.com/808041/diff/11001/12003#newcode302
src/pkg/nntp/nntp.go:302: }
On 2010/03/29 05:22:29, rsc wrote:
> lines?
> 

Done.

http://codereview.appspot.com/808041/diff/11001/12003#newcode442
src/pkg/nntp/nntp.go:442: return realres
On 2010/03/29 05:22:29, rsc wrote:
> Perhaps move these to the bottom of the file
> under a header
> 
> // Everything below this line is copied from package http.
> // It should be factored out into a common library.
> 

Done.

http://codereview.appspot.com/808041/diff/11001/12003#newcode597
src/pkg/nntp/nntp.go:597: goto Malformed
On 2010/03/29 05:22:29, rsc wrote:
> These all have specific codes that should come back
> (220 in this example).  Seems like it would be good
> practice to check for the specific code.
> 

Done.

http://codereview.appspot.com/808041/diff/11001/12003#newcode608
src/pkg/nntp/nntp.go:608: // Look for extension lines
On 2010/03/29 05:22:29, rsc wrote:
> a, err
> 
> Also, have you tested this? 
> c.readHeader calls c.body once, reading from it
> with a bufio.Reader, and then res.Body below
> calls c.body again.  Since the bufio.Reader is allowed
> to read beyond what is requested, the second c.body
> is probably missing some of the data.

I've changed the code to only call c.body() once and avoid the whole issue. I
also added a test case for checking that the body of the article arrives intact.

http://codereview.appspot.com/808041/diff/11001/12003#newcode663
src/pkg/nntp/nntp.go:663: 
On 2010/03/29 05:22:29, rsc wrote:
> prepend really means to ponder deeply.
> 
> easy to avoid: the string is a prefix anyway
> 
>  

Done.

http://codereview.appspot.com/808041/diff/11001/12004
File src/pkg/nntp/nntp_test.go (right):

http://codereview.appspot.com/808041/diff/11001/12004#newcode34
src/pkg/nntp/nntp_test.go:34: var lines = `101 Capability list:
On 2010/03/29 05:22:29, rsc wrote:
> move this and other big string out to top level
> (and maybe the bottom of the file) just for ease of readability.
> 
> var basicServer =
> var basicClient =
> 

Done.

[cemeyer, 2010-03-29 19:59:49]

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

Please take another look.

[rsc1, 2010-03-30 18:47:21]

CL = "change list", the thing "hg change" manipulates.  ;-)

[rsc1, 2010-04-02 07:44:11]

The code looks good.  It might help to try to
clarify message ids vs message numbers in a few places.
Does it makes sense to use int or int64 for message
numbers to distinguish them from the string ids?
Also, where in the protocol are they used?

Something, maybe the doc comment, should explain
the relationship between Conn, Group, and the ids.

http://codereview.appspot.com/808041/diff/18001/2004
File src/pkg/nntp/nntp.go (right):

http://codereview.appspot.com/808041/diff/18001/2004#newcode41
src/pkg/nntp/nntp.go:41: // A Conn represents a connection to an NNTP server.
This comment should probably be expanded to tell clients what
they need to know about the concepts involved in the methods.

// A Conn represents a connection to an NNTP server.

http://codereview.appspot.com/808041/diff/18001/2004#newcode49
src/pkg/nntp/nntp.go:49: // A Group represents the status of a group (as
returned by
// A Group gives information about a single news group on the server.

http://codereview.appspot.com/808041/diff/18001/2004#newcode54
src/pkg/nntp/nntp.go:54: // High and low message ids
message numbers?

I think of message ids as <123466@my.mail.server>

http://codereview.appspot.com/808041/diff/18001/2004#newcode57
src/pkg/nntp/nntp.go:57: // typical values are 'y', 'n', or 'm'.
change 'y' to "y" etc.

http://codereview.appspot.com/808041/diff/18001/2004#newcode193
src/pkg/nntp/nntp.go:193: func Dial(host string) (*Conn, os.Error) {
Maybe leave the network too?
Also make it clear that the port is required.

// Dial connects to an NNTP server.
// The network and addr are passed to net.Dial to
// make the connection.
//
// Example:
//    conn, err := nntp.Dial("tcp", "my.news:nntp")
//
func Dial(network, addr string) (*Conn, os.Error)

http://codereview.appspot.com/808041/diff/18001/2004#newcode241
src/pkg/nntp/nntp.go:241: // Authenticate sends AUTHINFO authentication
information to the
// Authenticate logs in to the NNTP server.
// It only sends the password if the server requires one.

http://codereview.appspot.com/808041/diff/18001/2004#newcode314
src/pkg/nntp/nntp.go:314: // NewNews returns a list of the IDs of articles
posted since the given time.
Is this specific to the current group?  If so, it should say
posted to the current group ...

http://codereview.appspot.com/808041/diff/18001/2004#newcode365
src/pkg/nntp/nntp.go:365: // Capabilities performs the CAPABILITIES command,
which (if the server
// Capabilities returns a list of the features supported by the server.

The methods above didn't say what the NNTP commands were,
and I think that is clearer.  Programmers who care about 
specific commands will be able to tell what they are.  ;-)
Instead, focus on what clients need to know to use them.
Specific suggestions scattered below.

http://codereview.appspot.com/808041/diff/18001/2004#newcode367
src/pkg/nntp/nntp.go:367: // 3977 for more information.
Don't need to mention 3977 in individual comments.
It's at the top.

http://codereview.appspot.com/808041/diff/18001/2004#newcode375
src/pkg/nntp/nntp.go:375: // Date performs the DATE command, which is used to
determine the server's
// Date returns the current time on the server.
// Typically the time is later passed to NewGroups or NewNews.

http://codereview.appspot.com/808041/diff/18001/2004#newcode390
src/pkg/nntp/nntp.go:390: // ListActive performs a LIST command, which returns a
list of active groups,
ListActive returns a list of the server's active groups.

Or maybe this command should go away in favor of
List("", "")?

http://codereview.appspot.com/808041/diff/18001/2004#newcode401
src/pkg/nntp/nntp.go:401: func (c *Conn) List(keyword, wildmat string)
([]string, os.Error) {
Maybe this should be ...string and then 

// List returns a list of groups present on the server.
// Valid forms are:
//
//     List()  - return the active groups
//     List(keyword)  - i have no idea
//     List(keyword, pattern)  - i have no idea
//

http://codereview.appspot.com/808041/diff/18001/2004#newcode415
src/pkg/nntp/nntp.go:415: // Group selects a group.
// Group changes the current group.

http://codereview.appspot.com/808041/diff/18001/2004#newcode441
src/pkg/nntp/nntp.go:441: // Help returns some sort of human-intelligable help.
// Help returns the server's help text.

http://codereview.appspot.com/808041/diff/18001/2004#newcode462
src/pkg/nntp/nntp.go:462: // Stat can be used to detect if an article exists.
The string results
Give the return values names for documentation.

Is the return id ever != id with err == nil?
If not, can drop and rely on os.Error for conveying errors.

// Stat looks up the message with the given id and
// returns its message number in the current group.
func (c *Conn) Stat(id string) (number string, err os.Error)

Why is number a string, by the way?

http://codereview.appspot.com/808041/diff/18001/2004#newcode470
src/pkg/nntp/nntp.go:470: // Last selects the previous article. The return
values and behavior are
// Last selects the previous article, returning its message number and id.
func (c *Conn) (id, number string, err os.Error)

http://codereview.appspot.com/808041/diff/18001/2004#newcode476
src/pkg/nntp/nntp.go:476: // Next selects the next article. The return values
and behavior are the
Same

http://codereview.appspot.com/808041/diff/18001/2004#newcode482
src/pkg/nntp/nntp.go:482: // RawArticle performs an ARTICLE command and returns
an io.Reader for
// RawArticle returns the article named by id as an io.Reader.
// The article is in plain text format, not NNTP wire format.

This isn't really raw.  Maybe it should be ArticleText
and similarly HeadText?

http://codereview.appspot.com/808041/diff/18001/2004#newcode492
src/pkg/nntp/nntp.go:492: // Article performs an ARTICLE command.
// Article returns the article named by id as an *Article.
// The Body field in the Article is valid only until the next
// call to a method of Conn.

http://codereview.appspot.com/808041/diff/18001/2004#newcode506
src/pkg/nntp/nntp.go:506: // RawHead performs a HEAD command, returning an
io.Reader for raw headers,
// RawHead returns the header for the article named by id as an io.Reader.
// The header is in plain text format, not NNTP wire format.

http://codereview.appspot.com/808041/diff/18001/2004#newcode515
src/pkg/nntp/nntp.go:515: // Head performs a HEAD command, parsing the headers.
// Head returns the header for the article named by id as an *Article.
// The Body field in the Article is nil.

http://codereview.appspot.com/808041/diff/18001/2004#newcode523
src/pkg/nntp/nntp.go:523: // Body performs a BODY command.
// Body returns the body for the article named by id as an io.Reader.

http://codereview.appspot.com/808041/diff/18001/2004#newcode531
src/pkg/nntp/nntp.go:531: // RawPost can be used to post articles. It reads from
r until EOF. Your post
RawPost reads a text-formatted article from r and posts it to the server.

http://codereview.appspot.com/808041/diff/18001/2004#newcode573
src/pkg/nntp/nntp.go:573: // Post posts an Article a. The same input
restrictions apply as for
// Post posts an article to the server.

[rsc1, 2010-04-02 07:48:23]

http://codereview.appspot.com/808041/diff/18001/2004
File src/pkg/nntp/nntp.go (right):

http://codereview.appspot.com/808041/diff/18001/2004#newcode165
src/pkg/nntp/nntp.go:165: func (a *Article) Read(p []byte) (int, os.Error) {
Should be commented, but is it necessary at all?
It's not clear to me what it does.
If it's just for WriteTo, it seems like it would be better to keep the reader
local to that function anyway (pass it directly to io.Copy).

[cemeyer, 2010-04-05 00:27:36]

Comments fixed up as suggested.

http://codereview.appspot.com/808041/diff/18001/2004
File src/pkg/nntp/nntp.go (right):

http://codereview.appspot.com/808041/diff/18001/2004#newcode41
src/pkg/nntp/nntp.go:41: // A Conn represents a connection to an NNTP server.
On 2010/04/02 07:44:11, rsc1 wrote:
> This comment should probably be expanded to tell clients what
> they need to know about the concepts involved in the methods.
> 
> // A Conn represents a connection to an NNTP server.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode49
src/pkg/nntp/nntp.go:49: // A Group represents the status of a group (as
returned by
On 2010/04/02 07:44:11, rsc1 wrote:
> // A Group gives information about a single news group on the server.

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode54
src/pkg/nntp/nntp.go:54: // High and low message ids
On 2010/04/02 07:44:11, rsc1 wrote:
> message numbers?
> 
> I think of message ids as <mailto:123466@my.mail.server>
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode57
src/pkg/nntp/nntp.go:57: // typical values are 'y', 'n', or 'm'.
On 2010/04/02 07:44:11, rsc1 wrote:
> change 'y' to "y" etc.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode165
src/pkg/nntp/nntp.go:165: func (a *Article) Read(p []byte) (int, os.Error) {
On 2010/04/02 07:48:23, rsc1 wrote:
> Should be commented, but is it necessary at all?
> It's not clear to me what it does.
> If it's just for WriteTo, it seems like it would be better to keep the reader
> local to that function anyway (pass it directly to io.Copy).

I'm thinking that callers may want to read from the article without having to
copy it into a buffer (with WriteTo) -- for example, a sort of "cut-through"
reposting would employ something like this. But I'm ok with nuking it, too, if
you think it's unhelpful.

Thinking about this a bit more later, I suppose callers who want this uncommon
behavior can implement their own Writer to do this. Nuking the function.

http://codereview.appspot.com/808041/diff/18001/2004#newcode193
src/pkg/nntp/nntp.go:193: func Dial(host string) (*Conn, os.Error) {
On 2010/04/02 07:44:11, rsc1 wrote:
> Maybe leave the network too?
> Also make it clear that the port is required.
> 
> // Dial connects to an NNTP server.
> // The network and addr are passed to net.Dial to
> // make the connection.
> //
> // Example:
> //    conn, err := nntp.Dial("tcp", "my.news:nntp")
> //
> func Dial(network, addr string) (*Conn, os.Error)

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode241
src/pkg/nntp/nntp.go:241: // Authenticate sends AUTHINFO authentication
information to the
On 2010/04/02 07:44:11, rsc1 wrote:
> // Authenticate logs in to the NNTP server.
> // It only sends the password if the server requires one.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode314
src/pkg/nntp/nntp.go:314: // NewNews returns a list of the IDs of articles
posted since the given time.
On 2010/04/02 07:44:11, rsc1 wrote:
> Is this specific to the current group?  If so, it should say
> posted to the current group ...
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode365
src/pkg/nntp/nntp.go:365: // Capabilities performs the CAPABILITIES command,
which (if the server
On 2010/04/02 07:44:11, rsc1 wrote:
> // Capabilities returns a list of the features supported by the server.
> 
> The methods above didn't say what the NNTP commands were,
> and I think that is clearer.  Programmers who care about 
> specific commands will be able to tell what they are.  ;-)
> Instead, focus on what clients need to know to use them.
> Specific suggestions scattered below.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode367
src/pkg/nntp/nntp.go:367: // 3977 for more information.
On 2010/04/02 07:44:11, rsc1 wrote:
> Don't need to mention 3977 in individual comments.
> It's at the top.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode375
src/pkg/nntp/nntp.go:375: // Date performs the DATE command, which is used to
determine the server's
On 2010/04/02 07:44:11, rsc1 wrote:
> // Date returns the current time on the server.
> // Typically the time is later passed to NewGroups or NewNews.
> 
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode390
src/pkg/nntp/nntp.go:390: // ListActive performs a LIST command, which returns a
list of active groups,
On 2010/04/02 07:44:11, rsc1 wrote:
> ListActive returns a list of the server's active groups.
> 
> Or maybe this command should go away in favor of
> List("", "")?
> 

(Done.)

I think I'm leaning towards this now -- I was thinking maybe the common, plain
"LIST" command might be useful separately, but I don't think it is common enough
to warrant another API function. Nuking it for now.

http://codereview.appspot.com/808041/diff/18001/2004#newcode401
src/pkg/nntp/nntp.go:401: func (c *Conn) List(keyword, wildmat string)
([]string, os.Error) {
On 2010/04/02 07:44:11, rsc1 wrote:
> Maybe this should be ...string and then 
> 
> // List returns a list of groups present on the server.
> // Valid forms are:
> //
> //     List()  - return the active groups
> //     List(keyword)  - i have no idea
> //     List(keyword, pattern)  - i have no idea
> //
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode415
src/pkg/nntp/nntp.go:415: // Group selects a group.
On 2010/04/02 07:44:11, rsc1 wrote:
> // Group changes the current group.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode441
src/pkg/nntp/nntp.go:441: // Help returns some sort of human-intelligable help.
On 2010/04/02 07:44:11, rsc1 wrote:
> // Help returns the server's help text.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode462
src/pkg/nntp/nntp.go:462: // Stat can be used to detect if an article exists.
The string results
On 2010/04/02 07:44:11, rsc1 wrote:
> Give the return values names for documentation.

Done.

> Is the return id ever != id with err == nil?
> If not, can drop and rely on os.Error for conveying errors.

STAT can take a message id or message number (just like ARTICLE, HEAD, and
BODY).

> // Stat looks up the message with the given id and
> // returns its message number in the current group.
> func (c *Conn) Stat(id string) (number string, err os.Error)
> 
> Why is number a string, by the way?

Number is a string because many of the methods fetching information related to
articles (Article, Head, Body, Stat) will take a message id or a message number,
and the common denominator type is string. The message "numbers" themselves
aren't especially meaningful, other than to impose an ordering for Next / Last.
The message referred to by a message number (even holding the selected group
constant) can vary from session to session.

http://codereview.appspot.com/808041/diff/18001/2004#newcode470
src/pkg/nntp/nntp.go:470: // Last selects the previous article. The return
values and behavior are
On 2010/04/02 07:44:11, rsc1 wrote:
> // Last selects the previous article, returning its message number and id.
> func (c *Conn) (id, number string, err os.Error)
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode476
src/pkg/nntp/nntp.go:476: // Next selects the next article. The return values
and behavior are the
On 2010/04/02 07:44:11, rsc1 wrote:
> Same
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode482
src/pkg/nntp/nntp.go:482: // RawArticle performs an ARTICLE command and returns
an io.Reader for
On 2010/04/02 07:44:11, rsc1 wrote:
> // RawArticle returns the article named by id as an io.Reader.
> // The article is in plain text format, not NNTP wire format.
> 
> This isn't really raw.  Maybe it should be ArticleText
> and similarly HeadText?
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode492
src/pkg/nntp/nntp.go:492: // Article performs an ARTICLE command.
On 2010/04/02 07:44:11, rsc1 wrote:
> // Article returns the article named by id as an *Article.
> // The Body field in the Article is valid only until the next
> // call to a method of Conn.
> 

Done. (Moved the bit about io.Reader only being valid until the next call to a
method of Conn to the Conn struct doc-comment.)

http://codereview.appspot.com/808041/diff/18001/2004#newcode506
src/pkg/nntp/nntp.go:506: // RawHead performs a HEAD command, returning an
io.Reader for raw headers,
On 2010/04/02 07:44:11, rsc1 wrote:
> // RawHead returns the header for the article named by id as an io.Reader.
> // The header is in plain text format, not NNTP wire format.
> 

Done. (Renamed to HeadText as suggested above.)

http://codereview.appspot.com/808041/diff/18001/2004#newcode515
src/pkg/nntp/nntp.go:515: // Head performs a HEAD command, parsing the headers.
On 2010/04/02 07:44:11, rsc1 wrote:
> // Head returns the header for the article named by id as an *Article.
> // The Body field in the Article is nil.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode523
src/pkg/nntp/nntp.go:523: // Body performs a BODY command.
On 2010/04/02 07:44:11, rsc1 wrote:
> // Body returns the body for the article named by id as an io.Reader.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode531
src/pkg/nntp/nntp.go:531: // RawPost can be used to post articles. It reads from
r until EOF. Your post
On 2010/04/02 07:44:11, rsc1 wrote:
> RawPost reads a text-formatted article from r and posts it to the server.
> 

Done.

http://codereview.appspot.com/808041/diff/18001/2004#newcode573
src/pkg/nntp/nntp.go:573: // Post posts an Article a. The same input
restrictions apply as for
On 2010/04/02 07:44:11, rsc1 wrote:
> // Post posts an article to the server.
> 

Done.

[cemeyer, 2010-04-05 00:28:06]

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

Please take another look.

[rsc1, 2010-04-05 02:42:44]

Looks pretty good.  Can the article field go?

http://codereview.appspot.com/808041/diff/24001/25003
File src/pkg/nntp/nntp.go (right):

http://codereview.appspot.com/808041/diff/24001/25003#newcode176
src/pkg/nntp/nntp.go:176: if a.reader == nil {
delete this line and the a.reader field,
so that clients can create their own Articles
(all exported fields)

http://codereview.appspot.com/808041/diff/24001/25003#newcode179
src/pkg/nntp/nntp.go:179: return io.Copy(w, a.reader)
return io.Copy(w, &articleReader{a: a})

[cemeyer, 2010-04-05 04:34:38]

By the article field, you mean Article.reader, right? It's been removed.

http://codereview.appspot.com/808041/diff/24001/25003
File src/pkg/nntp/nntp.go (right):

http://codereview.appspot.com/808041/diff/24001/25003#newcode176
src/pkg/nntp/nntp.go:176: if a.reader == nil {
On 2010/04/05 02:42:45, rsc1 wrote:
> delete this line and the a.reader field,
> so that clients can create their own Articles
> (all exported fields)
> 
> 

Done.

http://codereview.appspot.com/808041/diff/24001/25003#newcode179
src/pkg/nntp/nntp.go:179: return io.Copy(w, a.reader)
On 2010/04/05 02:42:45, rsc1 wrote:
> return io.Copy(w, &articleReader{a: a})
> 

Done.

[cemeyer, 2010-04-05 04:35:41]

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

Please take another look.

[rsc1, 2010-04-05 05:40:45]

LGTM

Thanks for working through with the review.

[rsc1, 2010-04-05 05:42:24]

Trying to apply the patch, I get this when I run gotest:

mini=; gotest
rm -f _test/nntp.a _gotest_.6
/Users/rsc/bin/6g -o _gotest_.6 nntp.go nntp_test.go
rm -f _test/nntp.a
/Users/rsc/bin/gopack grc _test/nntp.a _gotest_.6 
--- FAIL: nntp.TestBasic
	

FAIL
mini=; 

Any ideas?

Thanks.

[cemeyer, 2010-04-05 05:59:38]

On 2010/04/05 05:42:24, rsc1 wrote:
> Trying to apply the patch, I get this when I run gotest:
> 
> mini=; gotest
> rm -f _test/nntp.a _gotest_.6
> /Users/rsc/bin/6g -o _gotest_.6 nntp.go nntp_test.go
> rm -f _test/nntp.a
> /Users/rsc/bin/gopack grc _test/nntp.a _gotest_.6 
> --- FAIL: nntp.TestBasic
> 	
> 
> FAIL
> mini=; 
> 
> Any ideas?
> 
> Thanks.

Hm, I don't get this, but I haven't run all.bash in a while either. Ok, now I've
re-run all.bash and I don't get that error.

I do get another error because the tests assumed the date formatting stuff was
broken -- I'll fix that in a second.

[cemeyer, 2010-04-05 06:04:03]

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

Please take another look.

[rsc, 2010-04-05 06:23:53]

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

nntp: new package, NNTP client

R=rsc, rsc1
CC=golang-dev
http://codereview.appspot.com/808041

Committer: Russ Cox <rsc@golang.org>