Issue 75063: Adding validation for uniqueness of service_id X date in calendar_dates
DescriptionAdding two values with identical service_id and date into calendar_dates.txt does not make sense and the result is undefined. However, it is not prohibited by GTFS. This adds a warning if the duplicity on those two keys is found. Patch Set 1
Total comments: 2
Patch Set 2 : Changing single column name into tuple, adding test data.Patch Set 3 : Moving error handling into SetDateHasService.Patch Set 4 : Adding unit testPatch Set 5 : Adding unit test (from the right directory)
Total comments: 5
Patch Set 6 : Review turnaround
Total comments: 8
Patch Set 7 : Setting context of warningPatch Set 8 : Formatting
Total comments: 4
Patch Set 9 : Adding ClearContext()
Total comments: 2
Patch Set 10 : Removing unnecessary linebreakMessagesTotal messages: 13
I'm fine waiting until we have settled on an implemention before you add a test, but you'll need one before submitting. http://codereview.appspot.com/75063/diff/1/2 File python/transitfeed.py (right): http://codereview.appspot.com/75063/diff/1/2#newcode3822 Line 3822: service_id+' X '+row[1], I don't think this should use DuplicateID as is because column_name should be a literal column. Instead you need to generalize it so "ID" can mean a tuple of values or make a new type of error. It would also be good to check existing feeds for this problem. I've been trying to keep validation logic in the individual GenericGTFSObject subclasses but there doesn't seem to be an easy way to do it here. Maybe add more parameters to SetDateHasService that enable checking for existing dates?
Sign in to reply to this message.
Sorry, I didn't get to it for some time. > I'm fine waiting until we have settled on an implemention before you add a test, > but you'll need one before submitting. sure. But I'm afraid I have to add another test GTFS directory (the tested problem occurs only in GTFS, not in the internal data representation). > http://codereview.appspot.com/75063/diff/1/2 > File python/transitfeed.py (right): > > http://codereview.appspot.com/75063/diff/1/2#newcode3822 > Line 3822: service_id+' X '+row[1], > I don't think this should use DuplicateID as is because column_name should be a > literal column. Instead you need to generalize it so "ID" can mean a tuple of > values or make a new type of error. Done. > It would also be good to check existing feeds for this problem. which feeds? let's take this offline. > I've been trying to keep validation logic in the individual GenericGTFSObject > subclasses but there doesn't seem to be an easy way to do it here. well, that surprises me, looking at the bunch of problems reported in Loader :) Maybe add > more parameters to SetDateHasService that enable checking for existing dates? Ok, I did this. This way, I could get rid of the temporary dict.
Sign in to reply to this message.
Sorry for letting this slip through the cracks. I think it is still relevant. > But I'm afraid I have to add another test GTFS directory (the tested > problem occurs only in GTFS, not in the internal data representation). I'm not so keen on the huge directory tree for test data because it slows down svn checkout. You can avoid adding a directory by copying the pattern used in CalendarTxtIntegrationTestCase or StopBlankHeaders. http://codereview.appspot.com/75063/diff/7019/6021 File python/transitfeed.py (right): http://codereview.appspot.com/75063/diff/7019/6021#newcode178 Line 178: e = DuplicateID(column_names=column_names, values=values, this won't format very well. specifically existing messages will change from Duplicate ID foo in column column_foo. to something like Duplicate ID tuple(foo) in column tuple(column_foo). You will probably need to format the multiple column/value lists explicitly, either 1) here 2) in the caller, which would be similar to what you had in patch set 1 (sorry!). The issue I raised in response to that patch "column_name should be a literal column" is because feedvalidator uses the column_name to highlight the issue in the html output. 3) with a different problem type http://codereview.appspot.com/75063/diff/7019/6021#newcode2334 Line 2334: (service_id, date), how about using self.service_id instead of adding another parameter? The parameters of problems.DuplicateID( should be lined up to the right of the (
Sign in to reply to this message.
> I'm not so keen on the huge directory tree for test data because it slows down > svn checkout. You can avoid adding a directory by copying the pattern used in > CalendarTxtIntegrationTestCase or StopBlankHeaders. Cool, I didn't know about MemoryZipTestCase!
Sign in to reply to this message.
http://codereview.appspot.com/75063/diff/1/2 File python/transitfeed.py (right): http://codereview.appspot.com/75063/diff/1/2#newcode3822 Line 3822: service_id+' X '+row[1], On 2009/06/18 22:17:33, Tom wrote: > I don't think this should use DuplicateID as is because column_name should be a > literal column. Instead you need to generalize it so "ID" can mean a tuple of > values or make a new type of error. > It would also be good to check existing feeds for this problem. > I've been trying to keep validation logic in the individual GenericGTFSObject > subclasses but there doesn't seem to be an easy way to do it here. Maybe add > more parameters to SetDateHasService that enable checking for existing dates? Done. http://codereview.appspot.com/75063/diff/7019/6021#newcode178 Line 178: def UnusedStop(self, stop_id, stop_name, context=None): On 2009/10/20 23:06:24, Tom wrote: > this won't format very well. specifically existing messages will change from > Duplicate ID foo in column column_foo. > to something like > Duplicate ID tuple(foo) in column tuple(column_foo). > > You will probably need to format the multiple column/value lists explicitly, > either > 1) here I choose 1) because > 2) in the caller, which would be similar to what you had in patch set 1 > (sorry!). The issue I raised in response to that patch "column_name should be a > literal column" is because feedvalidator uses the column_name to highlight the > issue in the html output. It's better to preserve the structure information if we want to change the formatting sometimes. > 3) with a different problem type I think it should be the same problem type, it is actually duplicate ID, just the ID is not canonical. GOTO 366 http://codereview.appspot.com/75063/diff/7019/6021#newcode366 Line 366: The only issue is that the error message for tuple is: Duplicate ID (FULLW, 20100604) in column (service_id, date) but should rather be "... in columns ...". Is it a problem? I might change it into column(s) but maybe the correctness is not worth the confusion (in vast majority of cases). http://codereview.appspot.com/75063/diff/7019/6021#newcode2334 Line 2334: """Set the first day of service as a string in YYYYMMDD format""" On 2009/10/20 23:06:24, Tom wrote: > how about using self.service_id instead of adding another parameter? > > The parameters of problems.DuplicateID( should be lined up to the right of the ( Done.
Sign in to reply to this message.
On 2009/10/22 22:25:58, jirka wrote: > http://codereview.appspot.com/75063/diff/7019/6021#newcode366 > Line 366: > The only issue is that the error message for tuple is: > Duplicate ID (FULLW, 20100604) in column (service_id, date) > > but should rather be "... in columns ...". > Is it a problem? I might change it into column(s) but maybe the correctness is > not worth the confusion (in vast majority of cases). I'm not going to worry about ... in column ... but there is a bigger problem. Try duplicating a lines in test/data/good_feed/trips.txt and test/data/good_feed/calendar_dates.txt. The trips.txt error contains the file name and line number. The calendar_dates doesn't. This is because self._problems.SetFileContext(*context) isn't called before period.SetDateHasService. Your test should be able to check for context. http://codereview.appspot.com/75063/diff/11001/12001 File test/testtransitfeed.py (right): http://codereview.appspot.com/75063/diff/11001/12001#newcode3217 Line 3217: # Two lines with the same value of service_id and date.A badly formatted end_date used to generate an InvalidValue report from delete second sentence? http://codereview.appspot.com/75063/diff/11001/12002 File transitfeed.py (right): http://codereview.appspot.com/75063/diff/11001/12002#newcode2459 Line 2459: (self.service_id, date), This should be left-aligned with ('serv.... http://codereview.appspot.com/75063/diff/11001/12002#newcode2462 Line 2462: only one line between methods
Sign in to reply to this message.
On 2009/10/23 01:01:22, Tom wrote: > On 2009/10/22 22:25:58, jirka wrote: > > http://codereview.appspot.com/75063/diff/7019/6021#newcode366 > > Line 366: > > The only issue is that the error message for tuple is: > > Duplicate ID (FULLW, 20100604) in column (service_id, date) > > > > but should rather be "... in columns ...". > > Is it a problem? I might change it into column(s) but maybe the correctness is > > not worth the confusion (in vast majority of cases). > > I'm not going to worry about ... in column ... but there is a bigger problem. > Try duplicating a lines in test/data/good_feed/trips.txt and > test/data/good_feed/calendar_dates.txt. > The trips.txt error contains the file name and line number. The calendar_dates > doesn't. This is because self._problems.SetFileContext(*context) isn't called > before period.SetDateHasService. > Your test should be able to check for context. > > http://codereview.appspot.com/75063/diff/11001/12001 > File test/testtransitfeed.py (right): > > http://codereview.appspot.com/75063/diff/11001/12001#newcode3217 > Line 3217: # Two lines with the same value of service_id and date.A badly > formatted end_date used to generate an InvalidValue report from > delete second sentence? > > http://codereview.appspot.com/75063/diff/11001/12002 > File transitfeed.py (right): > > http://codereview.appspot.com/75063/diff/11001/12002#newcode2459 > Line 2459: (self.service_id, date), > This should be left-aligned with ('serv.... > > http://codereview.appspot.com/75063/diff/11001/12002#newcode2462 > Line 2462: > only one line between methods
Sign in to reply to this message.
http://codereview.appspot.com/75063/diff/11001/12001 File test/testtransitfeed.py (right): http://codereview.appspot.com/75063/diff/11001/12001#newcode3217 Line 3217: # Two lines with the same value of service_id and date.A badly formatted end_date used to generate an InvalidValue report from On 2009/10/23 01:01:22, Tom wrote: > delete second sentence? I still don't understand the second sentence. http://codereview.appspot.com/75063/diff/14006/13007 File python/transitfeed.py (right): http://codereview.appspot.com/75063/diff/14006/13007#newcode3993 Line 3993: continue else: periods[period.service_id] = (period, context) (from below) self._problems.ClearContext() http://codereview.appspot.com/75063/diff/14006/13007#newcode4022 Line 4022: self._problems.ClearContext()
Sign in to reply to this message.
Sorry, last time I replied without publishing the comments. http://codereview.appspot.com/75063/diff/11001/12001 File test/testtransitfeed.py (right): http://codereview.appspot.com/75063/diff/11001/12001#newcode3217 Line 3217: # Two lines with the same value of service_id and date.A badly formatted end_date used to generate an InvalidValue report from On 2009/10/23 01:01:22, Tom wrote: > delete second sentence? Done. http://codereview.appspot.com/75063/diff/11001/12001#newcode3217 Line 3217: # Two lines with the same value of service_id and date.A badly formatted end_date used to generate an InvalidValue report from On 2009/10/26 19:34:20, Tom wrote: > On 2009/10/23 01:01:22, Tom wrote: > > delete second sentence? > > I still don't understand the second sentence. It was just a copy paste error, did you check in the last Patch Set? http://codereview.appspot.com/75063/diff/11001/12002 File transitfeed.py (right): http://codereview.appspot.com/75063/diff/11001/12002#newcode2459 Line 2459: (self.service_id, date), On 2009/10/23 01:01:22, Tom wrote: > This should be left-aligned with ('serv.... Done. http://codereview.appspot.com/75063/diff/11001/12002#newcode2462 Line 2462: On 2009/10/23 01:01:22, Tom wrote: > only one line between methods Done.
Sign in to reply to this message.
http://codereview.appspot.com/75063/diff/14006/13007 File python/transitfeed.py (right): http://codereview.appspot.com/75063/diff/14006/13007#newcode3993 Line 3993: continue On 2009/10/26 19:34:20, Tom wrote: > else: > periods[period.service_id] = (period, context) (from below) > > self._problems.ClearContext() Done. http://codereview.appspot.com/75063/diff/14006/13007#newcode4022 Line 4022: On 2009/10/26 19:34:20, Tom wrote: > self._problems.ClearContext() Done.
Sign in to reply to this message.
One minor comment and this is good to submit. Please make sure CL refers to this review and http://code.google.com/p/googletransitdatafeed/issues/detail?id=82 You could also mark the issue fixed. Thanks! http://codereview.appspot.com/75063/diff/13011/14010 File python/transitfeed.py (right): http://codereview.appspot.com/75063/diff/13011/14010#newcode2456 Line 2456: problems=None): looks like it fits on previous line
Sign in to reply to this message.
http://codereview.appspot.com/75063/diff/13011/14010 File python/transitfeed.py (right): http://codereview.appspot.com/75063/diff/13011/14010#newcode2456 Line 2456: problems=None): On 2009/10/26 23:30:33, Tom wrote: > looks like it fits on previous line Done.
Sign in to reply to this message.
|
