FAQ
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/

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")
+ }
+}

Search Discussions

  • Bradfitz at Dec 11, 2012 at 5:13 pm
    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.go#newcode95
    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.go#newcode100
    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.

    https://codereview.appspot.com/6923049/
  • Rsc at Dec 11, 2012 at 5:30 pm
  • Rsc at Dec 11, 2012 at 5:36 pm
    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.


    https://codereview.appspot.com/6923049/
  • Rsc at Dec 11, 2012 at 5:39 pm
    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.go#newcode100
    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.

    https://codereview.appspot.com/6923049/
  • Brad Fitzpatrick at Dec 11, 2012 at 5:41 pm
    (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, 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/6923049/>

    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")
    + }
    +}

  • Ryan Slade at Dec 11, 2012 at 5:14 pm
    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 (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, 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/6923049/>

    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 at Dec 11, 2012 at 6:29 pm
    *** 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>


    https://codereview.appspot.com/6923049/
  • Ryanslade at Dec 11, 2012 at 6:37 pm

    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.


    https://codereview.appspot.com/6923049/
  • Ryanslade at Dec 11, 2012 at 6:44 pm
    Hello bradfitz@golang.org, rsc@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/6923049/
  • Ryanslade at Dec 11, 2012 at 7:37 pm

    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.

    https://codereview.appspot.com/6923049/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 11, '12 at 2:53p
activeDec 11, '12 at 7:37p
posts11
users3
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase