Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(898)

Issue 7489: Add test for ReadCsvDict, ignore whitespace before fields, warn about whitespace in header (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years, 5 months ago by Tom
Modified:
14 years, 8 months ago
CC:
googletransitdatafeed_googlegroups.com
Base URL:
http://googletransitdatafeed.googlecode.com/svn/trunk/
Visibility:
Public.

Description

Add a test for _ReadCsvDict In 1.1.6 _ReadCsvDict returned all whitespace in every field. _ReadCsv strips trailing and following space. Now _ReadCsvDict uses the csv module to skip initial whitespace that is before quotes. Using skipinitialspace is good because it fixes http://code.google.com/p/googletransitdatafeed/issues/detail?id=36 "Feed parsing doesn't deal with quoted values correctly when surrounded by spaces" Switching from stripping in _ReadCsv to inside the csv module is good because it lets the validator warn or error about fields that contain only quoted whitespace such as the middle field here myid, " ", "foo" _ReadCsvDict now warns if any field in the first row of a file starts or ends with whitespace after csv has skipped initial whitespace. This lets us detect whitespace inside quotes, though it is a rare problem. It adds a warning for people who have whitespace after a header field but of 675 gtfs files I found this in only 15 and only 2 partners had a file that put whitespace after every header field. When I move stop_times to use _ReadCsvDict this will fix http://code.google.com/p/googletransitdatafeed/issues/detail?id=97 I don't want to add tests for _ReadCsv if it is about to be deleted. It would be nicer to have validation inside csv parser.

Patch Set 1 #

Patch Set 2 : add test for quoted fields, TODO: add warning #

Patch Set 3 : svn update #

Patch Set 4 : strip whitespace only in header #

Patch Set 5 : whitespace and update comment about whitespace in existing feeds #

Unified diffs Side-by-side diffs Delta from patch set Stats (+223 lines, -9 lines) Patch
M python/test/testtransitfeed.py View 1 2 3 5 chunks +197 lines, -4 lines 0 comments Download
M python/transitfeed.py View 1 2 3 4 5 chunks +26 lines, -5 lines 1 comment Download

Messages

Total messages: 5
Tom
15 years, 5 months ago (2008-10-19 15:58:17 UTC) #1
chris.harrelson.code
http://codereview.appspot.com/7489/diff/10/201 File python/test/testtransitfeed.py (right): http://codereview.appspot.com/7489/diff/10/201#newcode147 Line 147: extra line http://codereview.appspot.com/7489/diff/10/204 File python/transitfeed.py (right): http://codereview.appspot.com/7489/diff/10/204#newcode3071 Line ...
15 years, 5 months ago (2008-10-21 21:33:31 UTC) #2
Tom
Lets try this again
15 years, 5 months ago (2008-10-29 18:49:17 UTC) #3
chris.harrelson.code
LGTM http://codereview.appspot.com/7489/diff/609/408 File python/transitfeed.py (right): http://codereview.appspot.com/7489/diff/609/408#newcode3151 Line 3151: "space characters.", clarify to mean only spaces ...
15 years, 5 months ago (2008-10-30 22:33:30 UTC) #4
chris.harrelson.code
15 years, 5 months ago (2008-10-30 22:33:31 UTC) #5
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b