code review 6870052: bufio: add ReadLines function

bufio: add ReadLines function

API addition proposal, similar to filepath.Walk.

Common enough pattern that I get bored of writing all the
time, reading lines from a file or other input.

[bradfitz, 2012-12-03 20:08:42]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2012-12-03 21:43:34]

On 2012/12/03 20:08:42, bradfitz wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://code.google.com/p/go

This looks reasonable to me. Are you stuck on the name ? Maybe EachLine or
WithLines might be a better name as this isn't reading anything, in the way an
io.Reader reads things.

[nigeltao, 2012-12-03 22:38:00]

FWIW, I like the general idea.

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:621: func ReadLines(r io.Reader, fn func(line string)
error) error {
s/string/[]byte/ ??

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:629: if err := fn(line); err != nil {
Maybe s/err/err1/ to be clear that it's a different variable.

[bradfitz, 2012-12-03 22:49:32]

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:621: func ReadLines(r io.Reader, fn func(line string)
error) error {
On 2012/12/03 22:38:00, nigeltao wrote:
> s/string/[]byte/ ??

I started with []byte but then once I saw that ReadBytes() was already doing
copies, I stopped caring.

I thought ReadBytes' common case was to return internal memory like ReadSlice
and ReadLine, and only allocate when a line was too long.  Once I saw that it
was always allocating, I figured returning a string was fine, and required less
docs.

I'd be fine making it []byte, but I'd want to document it to be more like
ReadSlice: just a view of internal data.  And then make that be the common case,
but only allocate when lines are too long.

Thoughts?

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:629: if err := fn(line); err != nil {
On 2012/12/03 22:38:00, nigeltao wrote:
> Maybe s/err/err1/ to be clear that it's a different variable.

Sure, will do if others like this proposal.

[nigeltao, 2012-12-03 22:52:20]

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:621: func ReadLines(r io.Reader, fn func(line string)
error) error {
On 2012/12/03 22:49:32, bradfitz wrote:
> I'd be fine making it []byte, but I'd want to document it to be more like
> ReadSlice: just a view of internal data.  And then make that be the common
case,
> but only allocate when lines are too long.
> 
> Thoughts?

SGTM.

[adg, 2012-12-03 23:22:26]

I don't personally care about this API. No objections, though.

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:628: if len(line) > 0 {
Why not report blank lines? some users might care.

[bradfitz, 2012-12-03 23:25:31]

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6870052/diff/2001/src/pkg/bufio/bufio.go#newco...
src/pkg/bufio/bufio.go:628: if len(line) > 0 {
On 2012/12/03 23:22:27, adg wrote:
> Why not report blank lines? some users might care.

That's not what this is about. "\n" is returned, but not a "" in ("", io.EOF).

[rog, 2012-12-04 11:43:22]

On 2012/12/03 20:08:42, bradfitz wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://code.google.com/p/go

I am not keen on this.

- ReadLines sounds like it might use ReadLine to get its lines (and hence
understand about \r too) but it doesn't.

- You almost always want to strip off the final newline, and this doesn't do
that for you.

- As with any iterator function, breaking out of the loop early is awkward - you
can't just call "break" or "return".


The main reason for this to exist is, as far as I can tell, the fact that
ReadLine is so awkward to use.

If reading lines was as simple as:

   for {
      line, err := b.ReadLine()
      if err != nil {
          break
      }
      doSomething(line)
   }

then would we consider ReadLines worth it?

Here are some aspects of the problem that the various methods in bufio.Reader
try to address:

1) Efficiency: can we read lines without incurring an allocation and copy for
each line?
2) Correctness: do we know if the final line is correctly terminated? Can we
deal with very long lines? Do we interpret \r\n terminators correctly?
3) Ease of use: how hard do we have to think when writing a loop that reads
lines?
4) Robustness: can a file with a very long line exhaust our memory?

For instance, ReadSlice does well on 1) and 4), but fails on 2) (it can't deal
with \r\n terminated lines) and 3) (it's awkward to use correctly).

I suggest that we can't have all these things together. I believe one particular
awkwardness comes from trying to deal with an unterminated final newline
correctly. ReadLine doesn't do so (there's no way of telling that the final
newline is unterminated) and I think that's a reasonable approach.

The other awkwardness comes from desiring efficiency in the usual case, but not
falling over on very long lines. We've got ReadSlice (and ReadLine) but if we
want to deal with long lines, we need to make the buffer at least as big as the
longest line we want to be able to read, and that's unnecessary overhead in the
common case. But if we use ReadString or ReadBytes, we're vulnerable to
maliciousness via large files with no line terminators.

Thinking about it a bit more, perhaps ReadLines *is* the way forward. Because
the line is an argument to the function, there is no expectation that it should
be ok to use after the function returns, so passing it a []byte returned from
ReadSlice or ReadLine seems just fine.

How about something like this?

// ReadLines reads lines from r and calls fn with each read line, not
// including the line terminator.  If a line exceeds the given maximum
// size, it will be truncated and the rest of the line discarded.  If fn
// returns a non-nil error, reading ends and the error is returned from
// ReadLines.  When EOF is encountered, ReadLines returns nil.
func ReadLines(r io.Reader, maxSize int, fn func(line []byte) error) error

http://play.golang.org/p/Y65r-cKEDX

It seems to tick most of the above boxes (you can't tell if you've got an
unterminated newline, but that fits with ReadLine's behaviour).

Open question: should a too-long line be discarded, or trigger an error?

[bradfitz, 2013-02-14 03:34:02]

Closed; moved to https://code.google.com/p/go/issues/detail?id=4802