code review 157168: Initial syslog module. It's my first go, so please bare...

Initial syslog module. It's my first go, so please bare with me for the ugliness.
Also making a small change to the log module, although it probably deserves a separate changelist with further error feedback fixes.

CONTRIBUTORS:
Yves Junqueira <yves.junqueira@gmail.com>

AUTHORS:
- N/A (I'm a Googler)

[nictuku, 2009-11-26 05:51:22]

Hello golang-dev@googlegroups.com,

I'd like you to review the following change.

[rsc, 2009-11-29 22:57:57]

Thanks for writing this.
A few design comments below, which I think will
simplify things a bit.

One procedural issue: you gave one email address
in the change description but used a different one
for logging into codereview.appspot.com.  We can
work around that for this one, but for future CLs
it helps us match code to contributor if these are
the same - would you prefer to use the gmail.com
one in the CONTRIBUTORS file or to set up a Google
Account for logging into codereview as yves@cetico.org?

Thanks.

http://codereview.appspot.com/157168/diff/2034/2035
File src/pkg/log/log.go (right):

http://codereview.appspot.com/157168/diff/2034/2035#newcode128
src/pkg/log/log.go:128: func (l *Logger) Output(calldepth int, s string) (err
os.Error) {
can drop the name here: just os.Error
is fine; the err name doesn't add anything.

http://codereview.appspot.com/157168/diff/2034/2035#newcode136
src/pkg/log/log.go:136: if err != nil {
I don't mind reporting the error, but an error
on out0 should not disable the write to out1
and it should also not stop the program from
exiting if that's what this logger does.

Please change this to

_, err := io.WriteString(l.out0, s);
if l.out1 != nil {
    _, err1 := io.WriteString(l.out1, s);
    if err == nil && err1 != nil {
        err = err1;
    }
}

then at the end

return err

http://codereview.appspot.com/157168/diff/2034/2037
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/157168/diff/2034/2037#newcode6
src/pkg/syslog/syslog.go:6: // the System Log service. It can send messages to
the
s/System Log/system log/

http://codereview.appspot.com/157168/diff/2034/2037#newcode7
src/pkg/syslog/syslog.go:7: // syslog daemon using UNIX sockets, or UDP and TCP
s/UNIX sockets/Unix domain sockets/

http://codereview.appspot.com/157168/diff/2034/2037#newcode8
src/pkg/syslog/syslog.go:8: // streams.
s/streams/connections/

http://codereview.appspot.com/157168/diff/2034/2037#newcode12
src/pkg/syslog/syslog.go:12: "io";
Please sort

http://codereview.appspot.com/157168/diff/2034/2037#newcode22
src/pkg/syslog/syslog.go:22: // From /usr/include/sys/syslog.h.
Add 
// These are the same on Linux, BSD, and OS X.

Also probably define

type Priority int

and then make this 

LOG_EMERG Priority = iota;

http://codereview.appspot.com/157168/diff/2034/2037#newcode33
src/pkg/syslog/syslog.go:33: // Syslog contains a message to be sent to the
server, as well as optional
I'm not sure it makes sense to expose all this mechanism.
I think it would suffice to do

// New establishes a new connection to the system log daemon.
// Each write to the returned writer sends a log message with
// the given priority and prefix.
func New(priority Priority, prefix string) (w io.WriteCloser, err os.Error)

// Dial establishes a connection to a log daemon by connecting
// to address raddr on the network net. 
// Each write to the returned writer sends a log message with
// the given priority and prefix.
func Dial(net, raddr string, priority Priority, prefix string) (w
io.WriteCloser, err os.Error)

and have both of these return a *log

type log struct {
    prefix []byte;
    conn net.Conn;
}

with these methods

func (l *log) Close() os.Error
func (l *log) Write(b []byte) (n int, err os.Error)

Write would just create a new buffer with room for
prefix and b, copy them both in, and write it to conn.

http://codereview.appspot.com/157168/diff/2034/2037#newcode50
src/pkg/syslog/syslog.go:50: p := strconv.Itoa(s.priority);
This can be just %d below.

In terms of the interface sketched above, this should be

prefix := strings.Bytes("<%d>%s: ", priority, prefix);

http://codereview.appspot.com/157168/diff/2034/2037#newcode51
src/pkg/syslog/syslog.go:51: m := string(message);
The conversion here is unnecessary.
You can pass message (perhaps rename it to m)
directly to sprintf and it will format as a string
when using %s.

http://codereview.appspot.com/157168/diff/2034/2037#newcode81
src/pkg/syslog/syslog.go:81: // TODO(yvesj): Re-use the connection.
Yes please - the interface sketched above makes this easy.

http://codereview.appspot.com/157168/diff/2034/2037#newcode172
src/pkg/syslog/syslog.go:172: // See that object documentation for details on
the parameters;
The object documentation is invisible because the
fields are hidden.  If you run "godoc syslog" you can
see what the docs look like for this object.

http://codereview.appspot.com/157168/diff/2034/2037#newcode181
src/pkg/syslog/syslog.go:181: func NewLogger(priority int) *log.Logger {
Maybe this should take two arguments

func NewLogger(p Priority, flag int)

and pass flag on to log.New, so that it is easy
to build loggers that crash or that include
the file and line number in the messages.

http://codereview.appspot.com/157168/diff/2034/2038
File src/pkg/syslog/syslog_test.go (right):

http://codereview.appspot.com/157168/diff/2034/2038#newcode6
src/pkg/syslog/syslog_test.go:6: // These tests are too heavyweight. I'm
providing them as to illustrate,
True but there should be tests.

http://codereview.appspot.com/157168/diff/2034/2038#newcode14
src/pkg/syslog/syslog_test.go:14: func TestSysLog(t *testing.T) {
This should be broken into multiple tests, probably.

TestNew

TestDial 

etc.

http://codereview.appspot.com/157168/diff/2034/2038#newcode15
src/pkg/syslog/syslog_test.go:15: // Simplest interface.
This test is writing to the system log, which probably
isn't too nice for an automated test.

Instead, maybe you should set up a simple UDP
listener (see the net or websocket unit tests for examples
of how to set up a listener in a test) and log to that.
Then also test that you can connect to the system
logger (i.e., that New succeeds) but don't bother
writing any messages to it.

[nictuku, 2009-12-05 23:54:47]

Hi rsc,

Thank you for the detailed review. 

Note that despite the lack of updates, I have not abandoned this change. I'm
fixing the code and I'll send replies to your comments soon.

Meanwhile, may I ask why did you suggest bytes[] for prefix inside the 'log'
structure? That is, instead of string..? string seemed a more n natural and
easier choice.

Thank you!
- Yves

On 2009/11/29 22:57:57, rsc wrote:
> Thanks for writing this.
> A few design comments below, which I think will
> simplify things a bit.
> 
> One procedural issue: you gave one email address
> in the change description but used a different one
> for logging into http://codereview.appspot.com.  We can
> work around that for this one, but for future CLs
> it helps us match code to contributor if these are
> the same - would you prefer to use the http://gmail.com
> one in the CONTRIBUTORS file or to set up a Google
> Account for logging into codereview as yves@cetico.org?
> 
> Thanks.
> 
> http://codereview.appspot.com/157168/diff/2034/2035
> File src/pkg/log/log.go (right):
> 
> http://codereview.appspot.com/157168/diff/2034/2035#newcode128
> src/pkg/log/log.go:128: func (l *Logger) Output(calldepth int, s string) (err
> os.Error) {
> can drop the name here: just os.Error
> is fine; the err name doesn't add anything.
> 
> http://codereview.appspot.com/157168/diff/2034/2035#newcode136
> src/pkg/log/log.go:136: if err != nil {
> I don't mind reporting the error, but an error
> on out0 should not disable the write to out1
> and it should also not stop the program from
> exiting if that's what this logger does.
> 
> Please change this to
> 
> _, err := io.WriteString(l.out0, s);
> if l.out1 != nil {
>     _, err1 := io.WriteString(l.out1, s);
>     if err == nil && err1 != nil {
>         err = err1;
>     }
> }
> 
> then at the end
> 
> return err
> 
> http://codereview.appspot.com/157168/diff/2034/2037
> File src/pkg/syslog/syslog.go (right):
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode6
> src/pkg/syslog/syslog.go:6: // the System Log service. It can send messages to
> the
> s/System Log/system log/
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode7
> src/pkg/syslog/syslog.go:7: // syslog daemon using UNIX sockets, or UDP and
TCP
> s/UNIX sockets/Unix domain sockets/
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode8
> src/pkg/syslog/syslog.go:8: // streams.
> s/streams/connections/
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode12
> src/pkg/syslog/syslog.go:12: "io";
> Please sort
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode22
> src/pkg/syslog/syslog.go:22: // From /usr/include/sys/syslog.h.
> Add 
> // These are the same on Linux, BSD, and OS X.
> 
> Also probably define
> 
> type Priority int
> 
> and then make this 
> 
> LOG_EMERG Priority = iota;
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode33
> src/pkg/syslog/syslog.go:33: // Syslog contains a message to be sent to the
> server, as well as optional
> I'm not sure it makes sense to expose all this mechanism.
> I think it would suffice to do
> 
> // New establishes a new connection to the system log daemon.
> // Each write to the returned writer sends a log message with
> // the given priority and prefix.
> func New(priority Priority, prefix string) (w io.WriteCloser, err os.Error)
> 
> // Dial establishes a connection to a log daemon by connecting
> // to address raddr on the network net. 
> // Each write to the returned writer sends a log message with
> // the given priority and prefix.
> func Dial(net, raddr string, priority Priority, prefix string) (w
> io.WriteCloser, err os.Error)
> 
> and have both of these return a *log
> 
> type log struct {
>     prefix []byte;
>     conn net.Conn;
> }
> 
> with these methods
> 
> func (l *log) Close() os.Error
> func (l *log) Write(b []byte) (n int, err os.Error)
> 
> Write would just create a new buffer with room for
> prefix and b, copy them both in, and write it to conn.
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode50
> src/pkg/syslog/syslog.go:50: p := strconv.Itoa(s.priority);
> This can be just %d below.
> 
> In terms of the interface sketched above, this should be
> 
> prefix := strings.Bytes("<%d>%s: ", priority, prefix);
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode51
> src/pkg/syslog/syslog.go:51: m := string(message);
> The conversion here is unnecessary.
> You can pass message (perhaps rename it to m)
> directly to sprintf and it will format as a string
> when using %s.
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode81
> src/pkg/syslog/syslog.go:81: // TODO(yvesj): Re-use the connection.
> Yes please - the interface sketched above makes this easy.
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode172
> src/pkg/syslog/syslog.go:172: // See that object documentation for details on
> the parameters;
> The object documentation is invisible because the
> fields are hidden.  If you run "godoc syslog" you can
> see what the docs look like for this object.
> 
> http://codereview.appspot.com/157168/diff/2034/2037#newcode181
> src/pkg/syslog/syslog.go:181: func NewLogger(priority int) *log.Logger {
> Maybe this should take two arguments
> 
> func NewLogger(p Priority, flag int)
> 
> and pass flag on to log.New, so that it is easy
> to build loggers that crash or that include
> the file and line number in the messages.
> 
> http://codereview.appspot.com/157168/diff/2034/2038
> File src/pkg/syslog/syslog_test.go (right):
> 
> http://codereview.appspot.com/157168/diff/2034/2038#newcode6
> src/pkg/syslog/syslog_test.go:6: // These tests are too heavyweight. I'm
> providing them as to illustrate,
> True but there should be tests.
> 
> http://codereview.appspot.com/157168/diff/2034/2038#newcode14
> src/pkg/syslog/syslog_test.go:14: func TestSysLog(t *testing.T) {
> This should be broken into multiple tests, probably.
> 
> TestNew
> 
> TestDial 
> 
> etc.
> 
> http://codereview.appspot.com/157168/diff/2034/2038#newcode15
> src/pkg/syslog/syslog_test.go:15: // Simplest interface.
> This test is writing to the system log, which probably
> isn't too nice for an automated test.
> 
> Instead, maybe you should set up a simple UDP
> listener (see the net or websocket unit tests for examples
> of how to set up a listener in a test) and log to that.
> Then also test that you can connect to the system
> logger (i.e., that New succeeds) but don't bother
> writing any messages to it.

[rsc, 2009-12-06 00:30:23]

On Sat, Dec 5, 2009 at 15:54,  <yves.junqueira@gmail.com> wrote:
> Meanwhile, may I ask why did you suggest bytes[] for prefix inside the
> 'log' structure? That is, instead of string..? string seemed a more n
> natural and easier choice.

ultimately the Writer takes []byte in its write call.
if you compute the []byte once then you don't
have the string -> []byte conversion every time
you use it.

(WriteString has such a conversion inside it)

[nictuku, 2009-12-07 18:43:51]

Please take another look.

http://codereview.appspot.com/157168/diff/2034/2035
File src/pkg/log/log.go (right):

http://codereview.appspot.com/157168/diff/2034/2035#newcode128
src/pkg/log/log.go:128: func (l *Logger) Output(calldepth int, s string) (err
os.Error) {
On 2009/11/29 22:57:57, rsc wrote:
> can drop the name here: just os.Error
> is fine; the err name doesn't add anything.
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2035#newcode136
src/pkg/log/log.go:136: if err != nil {
On 2009/11/29 22:57:57, rsc wrote:
> I don't mind reporting the error, but an error
> on out0 should not disable the write to out1
> and it should also not stop the program from
> exiting if that's what this logger does.
> 
> Please change this to
> 
> _, err := io.WriteString(l.out0, s);
> if l.out1 != nil {
>     _, err1 := io.WriteString(l.out1, s);
>     if err == nil && err1 != nil {
>         err = err1;
>     }
> }
> 
> then at the end
> 
> return err
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2037
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/157168/diff/2034/2037#newcode6
src/pkg/syslog/syslog.go:6: // the System Log service. It can send messages to
the
On 2009/11/29 22:57:57, rsc wrote:
> s/System Log/system log/
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2037#newcode7
src/pkg/syslog/syslog.go:7: // syslog daemon using UNIX sockets, or UDP and TCP
On 2009/11/29 22:57:57, rsc wrote:
> s/UNIX sockets/Unix domain sockets/
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2037#newcode8
src/pkg/syslog/syslog.go:8: // streams.
On 2009/11/29 22:57:57, rsc wrote:
> s/streams/connections/
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2037#newcode12
src/pkg/syslog/syslog.go:12: "io";
On 2009/11/29 22:57:57, rsc wrote:
> Please sort
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2037#newcode22
src/pkg/syslog/syslog.go:22: // From /usr/include/sys/syslog.h.
On 2009/11/29 22:57:57, rsc wrote:
> Add 
> // These are the same on Linux, BSD, and OS X.
> 
> Also probably define
> 
> type Priority int
> 
> and then make this 
> 
> LOG_EMERG Priority = iota;
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2037#newcode33
src/pkg/syslog/syslog.go:33: // Syslog contains a message to be sent to the
server, as well as optional
On 2009/11/29 22:57:57, rsc wrote:
> I'm not sure it makes sense to expose all this mechanism.
> I think it would suffice to do
> 
> // New establishes a new connection to the system log daemon.
> // Each write to the returned writer sends a log message with
> // the given priority and prefix.
> func New(priority Priority, prefix string) (w io.WriteCloser, err os.Error)
> 
> // Dial establishes a connection to a log daemon by connecting
> // to address raddr on the network net. 
> // Each write to the returned writer sends a log message with
> // the given priority and prefix.
> func Dial(net, raddr string, priority Priority, prefix string) (w
> io.WriteCloser, err os.Error)
> 
> and have both of these return a *log
> 
> type log struct {
>     prefix []byte;
>     conn net.Conn;
> }
> 
> with these methods
> 
> func (l *log) Close() os.Error
> func (l *log) Write(b []byte) (n int, err os.Error)
> 
> Write would just create a new buffer with room for
> prefix and b, copy them both in, and write it to conn.
> 

Done. Note that using []byte for prefix doesn't actually save any conversion. I
have to convert the whole thing to string so I can concatenate a "\n".  I would
keep it as string, but I'd rather hear what you think again.

http://codereview.appspot.com/157168/diff/2034/2037#newcode50
src/pkg/syslog/syslog.go:50: p := strconv.Itoa(s.priority);
On 2009/11/29 22:57:57, rsc wrote:
> This can be just %d below.
> 
> In terms of the interface sketched above, this should be
> 
> prefix := strings.Bytes("<%d>%s: ", priority, prefix);
> 

strings.Bytes doesn't do formatting, but I did a similar thing.

http://codereview.appspot.com/157168/diff/2034/2037#newcode51
src/pkg/syslog/syslog.go:51: m := string(message);
On 2009/11/29 22:57:57, rsc wrote:
> The conversion here is unnecessary.
> You can pass message (perhaps rename it to m)
> directly to sprintf and it will format as a string
> when using %s.
> 

Done. I got rid of this function and merged it into Write.

http://codereview.appspot.com/157168/diff/2034/2037#newcode81
src/pkg/syslog/syslog.go:81: // TODO(yvesj): Re-use the connection.
On 2009/11/29 22:57:57, rsc wrote:
> Yes please - the interface sketched above makes this easy.
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2037#newcode172
src/pkg/syslog/syslog.go:172: // See that object documentation for details on
the parameters;
On 2009/11/29 22:57:57, rsc wrote:
> The object documentation is invisible because the
> fields are hidden.  If you run "godoc syslog" you can
> see what the docs look like for this object.
> 

Deprecated this function in favor of Dial.

http://codereview.appspot.com/157168/diff/2034/2037#newcode181
src/pkg/syslog/syslog.go:181: func NewLogger(priority int) *log.Logger {
On 2009/11/29 22:57:57, rsc wrote:
> Maybe this should take two arguments
> 
> func NewLogger(p Priority, flag int)
> 
> and pass flag on to log.New, so that it is easy
> to build loggers that crash or that include
> the file and line number in the messages.

Done.

http://codereview.appspot.com/157168/diff/2034/2038
File src/pkg/syslog/syslog_test.go (right):

http://codereview.appspot.com/157168/diff/2034/2038#newcode6
src/pkg/syslog/syslog_test.go:6: // These tests are too heavyweight. I'm
providing them as to illustrate,
On 2009/11/29 22:57:57, rsc wrote:
> True but there should be tests.
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2038#newcode14
src/pkg/syslog/syslog_test.go:14: func TestSysLog(t *testing.T) {
On 2009/11/29 22:57:57, rsc wrote:
> This should be broken into multiple tests, probably.
> 
> TestNew
> 
> TestDial 
> 
> etc.
> 

Done.

http://codereview.appspot.com/157168/diff/2034/2038#newcode15
src/pkg/syslog/syslog_test.go:15: // Simplest interface.
On 2009/11/29 22:57:57, rsc wrote:
> This test is writing to the system log, which probably
> isn't too nice for an automated test.
> 
> Instead, maybe you should set up a simple UDP
> listener (see the net or websocket unit tests for examples
> of how to set up a listener in a test) and log to that.
> Then also test that you can connect to the system
> logger (i.e., that New succeeds) but don't bother
> writing any messages to it.
> 

Done.

[rsc, 2009-12-07 19:23:33]

http://codereview.appspot.com/157168/diff/5003/5006
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/157168/diff/5003/5006#newcode7
src/pkg/syslog/syslog.go:7: // syslog daemon using UNIX domain sockets, or UDP
and
s/or UDP and/UDP, or

http://codereview.appspot.com/157168/diff/5003/5006#newcode73
src/pkg/syslog/syslog.go:73: var p []byte;
I misled you before - saving prefix as a string is probably fine.

http://codereview.appspot.com/157168/diff/5003/5006#newcode106
src/pkg/syslog/syslog.go:106: if l.prefix == nil {
This can be

fmt.Fprintf(l.conn, "<%d>%s: %s\n", l.priority, l.prefix, b);

http://codereview.appspot.com/157168/diff/5003/5006#newcode116
src/pkg/syslog/syslog.go:116: // Emerg logs a message using the LOG_EMERG
priority.
These bother me, because they are editing shared state
(l.priority).  I think it would be better to drop these
and let programs create different syslogs for the 
different priorities.

Alternately, if these are important, define

func (l *syslog) writeString(p Priority, s string) os.Error {
    return fmt.Fprintf(l.conn, "<%d>%s: %s\n", p, l.prefix, s);
}

and then call that to implement each one.

http://codereview.appspot.com/157168/diff/5003/5006#newcode163
src/pkg/syslog/syslog.go:163: // priority will be used for all messages sent
using this interface.
// All messages are logged with priority p.

http://codereview.appspot.com/157168/diff/5003/5006#newcode164
src/pkg/syslog/syslog.go:164: // If you need more control, use 'New' or 'Dial'
instead.
can delete this sentence.

http://codereview.appspot.com/157168/diff/5003/5007
File src/pkg/syslog/syslog_test.go (right):

http://codereview.appspot.com/157168/diff/5003/5007#newcode10
src/pkg/syslog/syslog_test.go:10: //	"strings";
delete

[nictuku, 2009-12-07 20:24:32]

Thank you for the patience. Hopefully my further changes will be less demanding
of you.

http://codereview.appspot.com/157168/diff/5003/5006
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/157168/diff/5003/5006#newcode7
src/pkg/syslog/syslog.go:7: // syslog daemon using UNIX domain sockets, or UDP
and
On 2009/12/07 19:23:33, rsc wrote:
> s/or UDP and/UDP, or
> 

Done.

http://codereview.appspot.com/157168/diff/5003/5006#newcode73
src/pkg/syslog/syslog.go:73: var p []byte;
On 2009/12/07 19:23:33, rsc wrote:
> I misled you before - saving prefix as a string is probably fine.
> 

Done.

http://codereview.appspot.com/157168/diff/5003/5006#newcode106
src/pkg/syslog/syslog.go:106: if l.prefix == nil {
On 2009/12/07 19:23:33, rsc wrote:
> This can be
> 
> fmt.Fprintf(l.conn, "<%d>%s: %s\n", l.priority, l.prefix, b);
> 

Done. But moved the prefix check to Dial().

http://codereview.appspot.com/157168/diff/5003/5006#newcode116
src/pkg/syslog/syslog.go:116: // Emerg logs a message using the LOG_EMERG
priority.
On 2009/12/07 19:23:33, rsc wrote:
> These bother me, because they are editing shared state
> (l.priority).  I think it would be better to drop these
> and let programs create different syslogs for the 
> different priorities.
> 
> Alternately, if these are important, define
> 
> func (l *syslog) writeString(p Priority, s string) os.Error {
>     return fmt.Fprintf(l.conn, "<%d>%s: %s\n", p, l.prefix, s);
> }
> 
> and then call that to implement each one.

Done.

http://codereview.appspot.com/157168/diff/5003/5006#newcode163
src/pkg/syslog/syslog.go:163: // priority will be used for all messages sent
using this interface.
On 2009/12/07 19:23:33, rsc wrote:
> // All messages are logged with priority p.
> 

Done.

http://codereview.appspot.com/157168/diff/5003/5006#newcode164
src/pkg/syslog/syslog.go:164: // If you need more control, use 'New' or 'Dial'
instead.
On 2009/12/07 19:23:33, rsc wrote:
> can delete this sentence.
> 

Done.

http://codereview.appspot.com/157168/diff/5003/5007
File src/pkg/syslog/syslog_test.go (right):

http://codereview.appspot.com/157168/diff/5003/5007#newcode10
src/pkg/syslog/syslog_test.go:10: //	"strings";
On 2009/12/07 19:23:33, rsc wrote:
> delete
> 

Done.

[rsc, 2009-12-07 20:45:23]

http://codereview.appspot.com/157168/diff/5015/5018
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/157168/diff/5015/5018#newcode42
src/pkg/syslog/syslog.go:42: type SyslogWriter interface {
Final question: why is this an interface?
Will there be multiple implementations?
If not, it doesn't make sense to introduce
a new type when you could instead rename
type syslog to type Writer (clients would
use it as *syslog.Writer).

If this stays as an interface, it still doesn't need
a Syslog prefix, since the package name is already
syslog.  But I don't think you need the interface here.

[nictuku, 2009-12-08 03:12:04]

Thanks again.

http://codereview.appspot.com/157168/diff/5015/5018
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/157168/diff/5015/5018#newcode42
src/pkg/syslog/syslog.go:42: type SyslogWriter interface {
On 2009/12/07 20:45:24, rsc wrote:
> Final question: why is this an interface?
> Will there be multiple implementations?
> If not, it doesn't make sense to introduce
> a new type when you could instead rename
> type syslog to type Writer (clients would
> use it as *syslog.Writer).
> 
> If this stays as an interface, it still doesn't need
> a Syslog prefix, since the package name is already
> syslog.  But I don't think you need the interface here.
> 

I misinterpreted the documentation and thought that was the way to implement an
Interface - by extending it. Now I see that I only need to add the proper
functions.

Done.

[rsc, 2009-12-09 08:01:31]

looks good; one final change

http://codereview.appspot.com/157168/diff/5031/5034
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/157168/diff/5031/5034#newcode33
src/pkg/syslog/syslog.go:33: // WriteCloser implements io.WriteCloser and
provides other
WriteCloser is an interface name,
and a clumsy one at that.
This can be just Writer.

You don't need to say what it implements - let the
methods do that.

// A Writer is a connection to a syslog server.

[nictuku, 2009-12-09 16:28:33]

Done.

http://codereview.appspot.com/157168/diff/5031/5034
File src/pkg/syslog/syslog.go (right):

http://codereview.appspot.com/157168/diff/5031/5034#newcode33
src/pkg/syslog/syslog.go:33: // WriteCloser implements io.WriteCloser and
provides other
On 2009/12/09 08:01:31, rsc wrote:
> WriteCloser is an interface name,
> and a clumsy one at that.
> This can be just Writer.
> 
> You don't need to say what it implements - let the
> methods do that.
> 
> // A Writer is a connection to a syslog server.
> 

Done.

[rsc, 2009-12-11 20:41:54]

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

syslog: new package

R=golang-dev, rsc
http://codereview.appspot.com/157168

Committer: Russ Cox <rsc@golang.org>