adding feedvalidator_googletransit.py for using the extension directly

[neun, 2011-03-14 07:11:53]

Hi Andre,

this is a first proposal how we could create an executable for an extension. I
did refactor feedvalidator.main into two methods: one to read the options and
one to execute the validation. The new main in feedvalidator_googletransit now
just uses these two and injects the extension name.

Thanks for having a look at it,
Moritz

[anog, 2011-03-14 10:47:05]

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator.py
File python/feedvalidator.py (right):

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator.py#newcode618
python/feedvalidator.py:618: def ReadFeedAndOptions():
ParseCommandLineArguments? ParseArguments?

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator.py#newcode694
python/feedvalidator.py:694: """Validate feed, run in profiler if in options and
return an exit code."""
Please add a comma between "options" and "and", for readability.

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
File python/feedvalidator_googletransit.py (right):

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
python/feedvalidator_googletransit.py:17: # extension of feedvalidator.py using
the googletransit extension by default
This should be a module docstring, not a comment

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
python/feedvalidator_googletransit.py:19: 
Only one empty line, please

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
python/feedvalidator_googletransit.py:20: import extensions.googletransit
Unused import?

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
python/feedvalidator_googletransit.py:25: def main():
How about adding a complete integration test, like in testfeedvalidator.py, with
a good_feed_googletransit.zip?

Depending on how much test running time increases, maybe also run some of the
"bad" tests to ensure that those problems are also caught with this extension.
If good_feed.zip has nothing that clashes with this extension (ie it has no
errors/warnings with this extension) maybe run it too.

If the test running time increases significantly, then just run with a
known-good feed that uses extensions.

[neun, 2011-03-15 13:52:04]

Hi Andre,

thanks a lot for your suggestions. Here are my changes.

Best,
Moritz

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator.py
File python/feedvalidator.py (right):

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator.py#newcode618
python/feedvalidator.py:618: def ReadFeedAndOptions():
On 2011/03/14 10:47:05, anog wrote:
> ParseCommandLineArguments? ParseArguments?

Done.

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator.py#newcode694
python/feedvalidator.py:694: """Validate feed, run in profiler if in options and
return an exit code."""
On 2011/03/14 10:47:05, anog wrote:
> Please add a comma between "options" and "and", for readability.

Done.

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
File python/feedvalidator_googletransit.py (right):

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
python/feedvalidator_googletransit.py:17: # extension of feedvalidator.py using
the googletransit extension by default
On 2011/03/14 10:47:05, anog wrote:
> This should be a module docstring, not a comment

Done.

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
python/feedvalidator_googletransit.py:19: 
On 2011/03/14 10:47:05, anog wrote:
> Only one empty line, please

Done.

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
python/feedvalidator_googletransit.py:20: import extensions.googletransit
it's needed for py2exe, otherwise the extension code does not get included while
building the .exe

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
python/feedvalidator_googletransit.py:25: def main():
the running time increase seemed to be not really noticable. I added all "Full"
tests by inheriting FullTests from testfeedvalidator in
testgoogletransitextension

[anog, 2011-03-15 14:46:39]

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
File python/feedvalidator_googletransit.py (right):

http://codereview.appspot.com/4294044/diff/1/python/feedvalidator_googletrans...
python/feedvalidator_googletransit.py:20: import extensions.googletransit
On 2011/03/15 13:52:04, neun wrote:
> it's needed for py2exe, otherwise the extension code does not get included
while
> building the .exe

Oh, ok - thanks for adding the comment!

http://codereview.appspot.com/4294044/diff/6001/python/test/testgoogletransit...
File python/test/testgoogletransitextension.py (right):

http://codereview.appspot.com/4294044/diff/6001/python/test/testgoogletransit...
python/test/testgoogletransitextension.py:29: class
ExtensionFullTests(FullTests):
Please add a small docstring explaining what this is, so it's not necessary to
refer to testfeedvalidator.py.

http://codereview.appspot.com/4294044/diff/6001/python/test/testgoogletransit...
python/test/testgoogletransitextension.py:30: _FEEDVALIDATOR_PY =
'feedvalidator_googletransit.py'
Could you add a .zip file that makes use of the GTFS extensions supported by the
googletransit extension, and add a test that uses it here?

[neun, 2011-03-21 12:32:40]

Hi Andre,

thanks for your comments. I did the changes as suggested.

Best,
Moritz

http://codereview.appspot.com/4294044/diff/6001/python/test/testgoogletransit...
File python/test/testgoogletransitextension.py (right):

http://codereview.appspot.com/4294044/diff/6001/python/test/testgoogletransit...
python/test/testgoogletransitextension.py:29: class
ExtensionFullTests(FullTests):
On 2011/03/15 14:46:39, anog wrote:
> Please add a small docstring explaining what this is, so it's not necessary to
> refer to testfeedvalidator.py.

Done.

http://codereview.appspot.com/4294044/diff/6001/python/test/testgoogletransit...
python/test/testgoogletransitextension.py:30: _FEEDVALIDATOR_PY =
'feedvalidator_googletransit.py'
created new subdirectory "googletransit" to test/data and added good_feed using
feed_info and fare_attributes to it

[anog, 2011-03-21 15:43:49]

LGTM, just a small comment

http://codereview.appspot.com/4294044/diff/12001/python/test/testgoogletransi...
File python/test/testgoogletransitextension.py (right):

http://codereview.appspot.com/4294044/diff/12001/python/test/testgoogletransi...
python/test/testgoogletransitextension.py:44: htmlout =
open('validation-results.html').read()
Could you add a check to ensure that the extension name is included in the
output? (As in
http://code.google.com/p/googletransitdatafeed/source/detail?r=1497 )

[neun, 2011-03-22 09:56:59]

Thanks Andre!

I added the check. The new revision is
http://code.google.com/p/googletransitdatafeed/source/detail?r=1498