code review 6923049: encoding/csv: add Error method to Writer

encoding/csv: add Error method to Writer

Fixed issue 3931

[ryanslade, 2012-12-11 10:49:00]

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

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

[bradfitz, 2012-12-11 17:10:18]

(thinking aloud)

Could we "break" the Go 1 API contract and just an error return value to
Flush?

I can't imagine what that would break?  Maybe people's mock/testing code,
with a mock interface with a void Flush() function?


On Tue, Dec 11, 2012 at 5:49 AM, <ryanslade@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> encoding/csv: add Error method to Writer
>
> Fixed issue 3931
>
> Please review this at
https://codereview.appspot.**com/6923049/<https://codereview.appspot.com/6923...
>
> Affected files:
>   M src/pkg/encoding/csv/writer.go
>   M src/pkg/encoding/csv/writer_**test.go
>
>
> Index: src/pkg/encoding/csv/writer.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/csv/writer.**go
> +++ b/src/pkg/encoding/csv/writer.**go
> @@ -92,10 +92,17 @@
>  }
>
>  // Flush writes any buffered data to the underlying io.Writer.
> +// To check if an error occured during the Flush, call Error
>  func (w *Writer) Flush() {
>         w.w.Flush()
>  }
>
> +// Error can be called after Flush for clients who want to be careful
> +func (w *Writer) Error() error {
> +       _, err := w.w.Write(nil)
> +       return err
> +}
> +
>  // WriteAll writes multiple CSV records to w using Write and then calls
> Flush.
>  func (w *Writer) WriteAll(records [][]string) (err error) {
>         for _, record := range records {
> Index: src/pkg/encoding/csv/writer_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/encoding/csv/writer_**test.go
> +++ b/src/pkg/encoding/csv/writer_**test.go
> @@ -6,6 +6,7 @@
>
>  import (
>         "bytes"
> +       "errors"
>         "testing"
>  )
>
> @@ -42,3 +43,30 @@
>                 }
>         }
>  }
> +
> +type errorWriter struct{}
> +
> +func (e errorWriter) Write(b []byte) (int, error) {
> +       return 0, errors.New("Test")
> +}
> +
> +func TestError(t *testing.T) {
> +       b := &bytes.Buffer{}
> +       f := NewWriter(b)
> +       f.Write([]string{"abc"})
> +       f.Flush()
> +       err := f.Error()
> +
> +       if err != nil {
> +               t.Errorf("Unexpected error: %s\n", err)
> +       }
> +
> +       f = NewWriter(errorWriter{})
> +       f.Write([]string{"abc"})
> +       f.Flush()
> +       err = f.Error()
> +
> +       if err == nil {
> +               t.Error("Error should not be nil")
> +       }
> +}
>
>
>

[bradfitz, 2012-12-11 17:13:39]

https://codereview.appspot.com/6923049/diff/5001/src/pkg/encoding/csv/writer.go
File src/pkg/encoding/csv/writer.go (right):

https://codereview.appspot.com/6923049/diff/5001/src/pkg/encoding/csv/writer....
src/pkg/encoding/csv/writer.go:95: // To check if an error occured during the
Flush, call Error
end in a period.

https://codereview.appspot.com/6923049/diff/5001/src/pkg/encoding/csv/writer....
src/pkg/encoding/csv/writer.go:100: // Error can be called after Flush for
clients who want to be careful
If I buy safety glasses they don't say "You can wear these eye protection
glasses in your woodshop if you want to be careful".

How about just:

// Error returns the error from Flush.

Or:

// Error returns the Writer's last error.

[ryanslade, 2012-12-11 17:14:34]

To be honest, I was thinking the same thing.

The only people likely to use the new Error() method could just as easily
check for a return value in a new Flush() method.

And, as you say, I can't see any scenarios where adding a return value
would break existing code.


On Tue, Dec 11, 2012 at 5:10 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> (thinking aloud)
>
> Could we "break" the Go 1 API contract and just an error return value to
> Flush?
>
> I can't imagine what that would break?  Maybe people's mock/testing code,
> with a mock interface with a void Flush() function?
>
>
>
> On Tue, Dec 11, 2012 at 5:49 AM, <ryanslade@gmail.com> wrote:
>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go/
>>
>>
>> Description:
>> encoding/csv: add Error method to Writer
>>
>> Fixed issue 3931
>>
>> Please review this at
https://codereview.appspot.**com/6923049/<https://codereview.appspot.com/6923...
>>
>> Affected files:
>>   M src/pkg/encoding/csv/writer.go
>>   M src/pkg/encoding/csv/writer_**test.go
>>
>>
>> Index: src/pkg/encoding/csv/writer.go
>> ==============================**==============================**=======
>> --- a/src/pkg/encoding/csv/writer.**go
>> +++ b/src/pkg/encoding/csv/writer.**go
>> @@ -92,10 +92,17 @@
>>  }
>>
>>  // Flush writes any buffered data to the underlying io.Writer.
>> +// To check if an error occured during the Flush, call Error
>>  func (w *Writer) Flush() {
>>         w.w.Flush()
>>  }
>>
>> +// Error can be called after Flush for clients who want to be careful
>> +func (w *Writer) Error() error {
>> +       _, err := w.w.Write(nil)
>> +       return err
>> +}
>> +
>>  // WriteAll writes multiple CSV records to w using Write and then calls
>> Flush.
>>  func (w *Writer) WriteAll(records [][]string) (err error) {
>>         for _, record := range records {
>> Index: src/pkg/encoding/csv/writer_**test.go
>> ==============================**==============================**=======
>> --- a/src/pkg/encoding/csv/writer_**test.go
>> +++ b/src/pkg/encoding/csv/writer_**test.go
>> @@ -6,6 +6,7 @@
>>
>>  import (
>>         "bytes"
>> +       "errors"
>>         "testing"
>>  )
>>
>> @@ -42,3 +43,30 @@
>>                 }
>>         }
>>  }
>> +
>> +type errorWriter struct{}
>> +
>> +func (e errorWriter) Write(b []byte) (int, error) {
>> +       return 0, errors.New("Test")
>> +}
>> +
>> +func TestError(t *testing.T) {
>> +       b := &bytes.Buffer{}
>> +       f := NewWriter(b)
>> +       f.Write([]string{"abc"})
>> +       f.Flush()
>> +       err := f.Error()
>> +
>> +       if err != nil {
>> +               t.Errorf("Unexpected error: %s\n", err)
>> +       }
>> +
>> +       f = NewWriter(errorWriter{})
>> +       f.Write([]string{"abc"})
>> +       f.Flush()
>> +       err = f.Error()
>> +
>> +       if err == nil {
>> +               t.Error("Error should not be nil")
>> +       }
>> +}
>>
>>
>>
>

[rsc, 2012-12-11 17:16:30]

I understand that it's tempting to break the API contract, and also it is
unlikely that things will change, but part of the contract is that we don't get
to make that call. We don't know about the closed source uses of Go.

https://codereview.appspot.com/6923049/diff/5001/src/pkg/encoding/csv/writer.go
File src/pkg/encoding/csv/writer.go (right):

https://codereview.appspot.com/6923049/diff/5001/src/pkg/encoding/csv/writer....
src/pkg/encoding/csv/writer.go:100: // Error can be called after Flush for
clients who want to be careful
// Error reports any error that has occurred during a previous Write or Flush.

[ryanslade, 2012-12-11 17:27:30]

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

Please take another look.

[ryanslade, 2012-12-11 17:28:32]

On 2012/12/11 17:27:30, ryanslade wrote:
> Hello mailto:bradfitz@golang.org, mailto:rsc@golang.org (cc:
mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

I've made changes to the comments as suggested.

[rsc, 2012-12-11 17:30:13]

LGTM

[rsc, 2012-12-11 17:31:01]

It looks like you need to complete a CLA as described at
http://golang.org/doc/contribute.html#copyright

Please do that and then I will submit this CL.

[ryanslade, 2012-12-11 17:39:52]

On 2012/12/11 17:31:01, rsc wrote:
> It looks like you need to complete a CLA as described at
> http://golang.org/doc/contribute.html#copyright
> 
> Please do that and then I will submit this CL.

I've submitted a CLA.

[rsc, 2012-12-11 18:29:27]

*** Submitted as https://code.google.com/p/go/source/detail?r=91e7645fae10 ***

encoding/csv: add Error method to Writer

Fixed issue 3931

R=bradfitz, rsc
CC=golang-dev
https://codereview.appspot.com/6923049

Committer: Russ Cox <rsc@golang.org>