code review 12294043: encoding/csv: always allow trailing commas

encoding/csv: always allow trailing commas

Original CL by rsc (11916045):

The motivation for disallowing them was RFC 4180 saying
"The last field in the record must not be followed by a comma."
I believe this is an admonition to CSV generators, not readers.
When reading, anything followed by a comma is not the last field.

Fixes issue 5892.

[peted, 2013-08-01 23:39:53]

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

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

[rsc, 2013-08-02 16:58:21]

https://codereview.appspot.com/12294043/diff/6001/src/pkg/encoding/csv/reader.go
File src/pkg/encoding/csv/reader.go (right):

https://codereview.appspot.com/12294043/diff/6001/src/pkg/encoding/csv/reader...
src/pkg/encoding/csv/reader.go:257: r1, err := r.readRune()
Thanks for finding the bug here. The fix you've made duplicates an existing code
path. This structure should make the code clearer and more obviously correct:

r1, err := r.readRune()
for err == nil && r.TrimLeadingSpace && r1 != '\n' && unicode.IsSpace(r1) {
    r1, err = r.readRune()
}

if err == io.EOF && r.column != 0 {
    return true, 0, err
}
if err != nil {
    return false, 0, err
}

switch r1 { 
..

https://codereview.appspot.com/12294043/diff/6001/src/pkg/encoding/csv/reader...
src/pkg/encoding/csv/reader.go:273: } else {
no else after if body ending in return
but this is moot

[peted, 2013-08-02 18:58:48]

CL updated.

https://codereview.appspot.com/12294043/diff/6001/src/pkg/encoding/csv/reader.go
File src/pkg/encoding/csv/reader.go (right):

https://codereview.appspot.com/12294043/diff/6001/src/pkg/encoding/csv/reader...
src/pkg/encoding/csv/reader.go:257: r1, err := r.readRune()
On 2013/08/02 16:58:22, rsc wrote:
> Thanks for finding the bug here. The fix you've made duplicates an existing
code
> path. This structure should make the code clearer and more obviously correct:
> 
> r1, err := r.readRune()
> for err == nil && r.TrimLeadingSpace && r1 != '\n' && unicode.IsSpace(r1) {
>     r1, err = r.readRune()
> }
> 
> if err == io.EOF && r.column != 0 {
>     return true, 0, err
> }
> if err != nil {
>     return false, 0, err
> }
> 
> switch r1 { 
> ..

Done.

https://codereview.appspot.com/12294043/diff/6001/src/pkg/encoding/csv/reader...
src/pkg/encoding/csv/reader.go:273: } else {
On 2013/08/02 16:58:22, rsc wrote:
> no else after if body ending in return
> but this is moot

Done.

[peted, 2013-08-02 18:59:42]

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

Please take another look.

[peted, 2013-08-08 09:35:34]

On 2013/08/02 18:59:42, peted wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:rsc@golang.org (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Is this still a desired change?

[r, 2013-08-09 05:45:27]

LGTM

[r, 2013-08-09 05:46:08]

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

encoding/csv: always allow trailing commas

Original CL by rsc (11916045):

The motivation for disallowing them was RFC 4180 saying
"The last field in the record must not be followed by a comma."
I believe this is an admonition to CSV generators, not readers.
When reading, anything followed by a comma is not the last field.

Fixes issue 5892.

R=golang-dev, rsc, r
CC=golang-dev
https://codereview.appspot.com/12294043

Committer: Rob Pike <r@golang.org>