Issue 320: Refactor stop_timezone validation code out of google exten…

…sions and into main validator

Reviewed at http://codereview.appspot.com/5641063/

extensions/googletransit/stop.py

@@ -17,8 +17,6 @@
 import transitfeed
 import transitfeed.util as util
 import transitfeed.problems as problems_module
-import route as route_extension
-import re
 
 class Stop(transitfeed.Stop):
   """Extension of transitfeed.Stop:
@@ -28,16 +26,11 @@ class Stop(transitfeed.Stop):
   - Overriding ValidateStopLocationType(), adding location_type 2 (entrance).
   """
 
-  _FIELD_NAMES = transitfeed.Stop._FIELD_NAMES + ['stop_timezone',
-                                                  'vehicle_type',
+  _FIELD_NAMES = transitfeed.Stop._FIELD_NAMES + ['vehicle_type',
                                                   'wheelchair_boarding']
 
   LOCATION_TYPE_ENTRANCE = 2
 
-  # New validation function for field 'stop_timezone'.
-  def ValidateStopTimezone(self, problems):
-    util.ValidateTimezone(self.stop_timezone, 'stop_timezone', problems)
-
   # New validation function for field 'vehicle_type'.
   def ValidateVehicleType(self, problems):
     self.vehicle_type = util.ValidateAndReturnIntValue(
@@ -63,7 +56,6 @@ def ValidateWheelchairBoarding(self, problems):
   # Overriding transitfeed.Stop.ValidateBeforeAdd().
   def ValidateBeforeAdd(self, problems):
     super(Stop, self).ValidateBeforeAdd(problems)
-    self.ValidateStopTimezone(problems)
     self.ValidateVehicleType(problems)
     self.ValidateWheelchairBoarding(problems)
     return True # None of these checks are blocking
@@ -77,14 +69,6 @@ def ValidateStopLocationType(self, problems):
     if self.location_type == 2 and util.IsEmpty(self.parent_station):
       problems.InvalidValue('location_type', self.location_type,
           reason='an entrance must have a parent_station')
-    # Entrances or other child stops (having a parent station) must not have a
-    # stop_timezone.
-    if (not util.IsEmpty(self.parent_station) and
-        not util.IsEmpty(self.stop_timezone)):
-      problems.InvalidValue('location_type', self.location_type,
-          reason='an entrance or a stop having a parent stop must not have a '
-          'stop_timezone', type=problems_module.TYPE_WARNING)
-
 
   # Overriding _ReportMissingRequiredField() in order to allow empty stop_name
   # if location_type=2 (entrance).

test/testgoogletransitextension.py

@@ -219,12 +219,6 @@ def setUp(self):
     self._entrance.parent_station = self._parent_stop.stop_id
     self._entrance._gtfs_factory = self.gtfs_factory
 
-  def testValidateStopTimezone(self):
-    self._stop.stop_timezone = 'TestTimeZone/Wrong_Place'
-    self._stop.Validate(self.problems)
-    e = self.accumulator.PopInvalidValue('stop_timezone')
-    self.accumulator.AssertNoMoreExceptions()
-
   def testValidateVehicleType(self):
     # Test with non-integer value
     self._stop.vehicle_type = 'abc'
@@ -246,7 +240,7 @@ def testEntranceExceptions(self):
     # An entrance must not have a stop_timezone
     self._entrance.stop_timezone = 'America/Los_Angeles'
     self._entrance.Validate(self.problems)
-    e = self.accumulator.PopInvalidValue('location_type')
+    e = self.accumulator.PopInvalidValue('stop_timezone')
     self.assertMatchesRegex(r'stop_timezone', e.FormatProblem())
     self.accumulator.AssertNoMoreExceptions()
     self._entrance.stop_timezone = None
@@ -273,7 +267,7 @@ def testChildExceptions(self):
     # A _child_stop must not have a stop_timezone
     self._child_stop.stop_timezone = 'America/Los_Angeles'
     self._child_stop.Validate(self.problems)
-    e = self.accumulator.PopInvalidValue('location_type')
+    e = self.accumulator.PopInvalidValue('stop_timezone')
     self.assertMatchesRegex(r'stop_timezone', e.FormatProblem())
     self.assertTrue(e.IsWarning())
     self.accumulator.AssertNoMoreExceptions()

test/testtransitfeed.py

@@ -1119,6 +1119,12 @@ def runTest(self):
     self.ValidateAndExpectInvalidValue(stop, 'stop_desc')
     stop.stop_desc = 'Edge of the Couch'
     self.accumulator.AssertNoMoreExceptions()
+
+    stop.stop_timezone = 'This_Timezone/Does_Not_Exist'
+    self.ValidateAndExpectInvalidValue(stop, 'stop_timezone')
+    stop.stop_timezone = 'America/Los_Angeles'
+    stop.Validate(self.problems)
+    self.accumulator.AssertNoMoreExceptions()
 
 
 class StopAttributes(ValidationTestCase):
@@ -1930,6 +1936,23 @@ def testStopTooFarFromParentStation(self):
         " station Bullfrog (ID BULLFROG_ST)") != -1)
     self.accumulator.AssertNoMoreExceptions()
 
+  def testStopTimeZone(self):
+    self.SetArchiveContents(
+        "stops.txt",
+        "stop_id,stop_name,stop_lat,stop_lon,location_type,parent_station,"
+        "stop_timezone\n"
+        "BEATTY_AIRPORT,Airport,36.868446,-116.784582,,STATION,"
+        "America/New_York\n"
+        "STATION,Airport,36.868446,-116.784582,1,,\n"
+        "BULLFROG,Bullfrog,36.88108,-116.81797,,,\n"
+        "STAGECOACH,Stagecoach Hotel,36.915682,-116.751677,,,\n")    
+    self.MakeLoaderAndLoad()
+    e = self.accumulator.PopException("InvalidValue")
+    self.assertEqual(1, e.type)  # Warning
+    self.assertEquals(2, e.row_num)
+    self.assertEquals("stop_timezone", e.column_name)
+    self.accumulator.AssertNoMoreExceptions()
+
   #Uncomment once validation is implemented
   #def testStationWithoutReference(self):
   #  self.SetArchiveContents(

transitfeed/stop.py

@@ -38,7 +38,7 @@ class Stop(GtfsObjectBase):
   _REQUIRED_FIELD_NAMES = ['stop_id', 'stop_name', 'stop_lat', 'stop_lon']
   _FIELD_NAMES = _REQUIRED_FIELD_NAMES + \
                  ['stop_desc', 'zone_id', 'stop_url', 'stop_code',
-                  'location_type', 'parent_station']
+                  'location_type', 'parent_station', 'stop_timezone']
   _TABLE_NAME = 'stops'
 
   LOCATION_TYPE_STATION = 1
@@ -221,6 +221,16 @@ def ValidateStopIsNotStationWithParent(self, problems):
                             'Stop row with location_type=1 (a station) must '
                             'not have a parent_station')
 
+  def ValidateStopTimezone(self, problems):
+    # Entrances or other child stops (having a parent station) must not have a
+    # stop_timezone.
+    util.ValidateTimezone(self.stop_timezone, 'stop_timezone', problems)
+    if (not util.IsEmpty(self.parent_station) and
+        not util.IsEmpty(self.stop_timezone)):
+      problems.InvalidValue('stop_timezone', self.stop_timezone,
+          reason='a stop having a parent stop must not have a stop_timezone',
+          type=problems_module.TYPE_WARNING)
+
   def ValidateBeforeAdd(self, problems):
     # First check that all required fields are present because ParseAttributes
     # may remove invalid attributes.
@@ -233,6 +243,7 @@ def ValidateBeforeAdd(self, problems):
     self.ValidateStopLongitude(problems)
     self.ValidateStopUrl(problems)
     self.ValidateStopLocationType(problems)
+    self.ValidateStopTimezone(problems)
 
     # Check that this object is consistent with itself
     self.ValidateStopNotTooCloseToOrigin(problems)