Code review comment for lp:~themue/juju-core/057-tailer

Please take a look.

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer.go
File utils/tailer/tailer.go (right):

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer.go#newcode49
utils/tailer/tailer.go:49: func NewTailer(readSeeker io.ReadSeeker,
writeCloser io.WriteCloser, lines int, filter TailerFilterFunc) *Tailer
{
On 2013/12/12 14:46:39, rog wrote:
> I don't think there's any particular reason we want to give this code
the
> responsibility for closing the writer. It's quite possible we might
have several
> tailers writing to the same writer, for example.

> Better would be a Wait method (and perhaps a Dead method) to wait
until
> it's finished. Then the caller has the option of closing the writer.

Done as discussed.

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go
File utils/tailer/tailer_test.go (right):

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode40
utils/tailer/tailer_test.go:40: {
On 2013/12/12 14:46:39, rog wrote:
> If we move this brace onto the previous line, we can save a level of
indentation
> in all these tests.

Done.

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode42
utils/tailer/tailer_test.go:42: data: data[26:29],
On 2013/12/12 14:46:39, rog wrote:
> I like the new tests much better now. One thought though:

> I'm not convinced that referring to "data" in all these tests makes
them more
> readable - I have no idea what's in data[26:29] without counting or
using grep
> -n.

> Something like the below shows me exactly what's going
> on without me needing to refer to multiple places,
> and is independent of any changes else in the code.

> It would make it considerably easier for me to scan down
> the tests to ensure that each one represents reasonable
> behaviour.

> {
> description: "lines are longer than buffer size",
> data: []string{
> "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz\n",
> "0123456789012345678901234567890123456789012345678901\n",
> "the quick brown fox ",
> },
> initialLinesWritten: 1,
> initialLinesRequested: 1,
> bufferSize: 5,
> initialCollectedData: []string{
> "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyz\n",
> },
> appendedCollectedData: []string{
> "0123456789012345678901234567890123456789012345678901\n",
> },
> },

This case the test table would blow up again and also would be
inconsistent (e.g. when using the standard larger group of lines). So
now using three different data variables with more speaking names as a
compromise. Also located them near to the table.

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode215
utils/tailer/tailer_test.go:215: line, err := buffer.ReadString('\n')
On 2013/12/12 14:46:39, rog wrote:
> If the tailer doesn't produce enough data, we'll block here forever I
think,
> regardless of the timeout.

> I think you probably want something more like the below:

> func assertCollected(c *gc.C, reader io.Reader, compare []string,
injection
> func([]string)
> ) {
> lineChan := make(chan string)
> go func() {
> defer close(lineChan)
> reader := bufio.NewReader(reader)
> for {
> line, err := reader.ReadString('\n')
> if !c.Check(err, gc.IsNil) {
> return
> }
> lineChan <- line
> }
> }()
> timeout := time.After(testing.LongWait)
> for {
> select {
> case line, ok := <-lineChan:
> c.Assert(ok, jc.IsTrue)
> deal with line as currently
> case <-timeout:
> timed out
> }
> }
> }

Done with external reading goroutine.

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode222
utils/tailer/tailer_test.go:222: for i := 0; i < len(lines); i++ {
On 2013/12/12 14:46:39, rog wrote:
> c.Assert(lines, gc.DeepEquals, compare) ?

Done.

https://codereview.appspot.com/36540043/diff/80001/utils/tailer/tailer_test.go#newcode228
utils/tailer/tailer_test.go:228: if err == io.EOF {
On 2013/12/12 14:46:39, rog wrote:
> Once we've got EOF once on a pipe, I don't think we can get any more
data, can
> we?

Yeah, missed it when moving from buffer to Pipe.

https://codereview.appspot.com/36540043/