Allow schedule source to be specified

[pantierra]

Proposed solution for #80.

• Renamed (transitfeed) schedule object to feed variable name in osm2gtfs
• Introduced a schedule_source in the configuration file. Can be either a local path, a url to download any file to be used in the creators to get the time information in, or a "None" (like in Accra) to omit processing of any schedule file.
• Introduced a --refresh-schedule-source argument to re-download a cached schedule file.
• Introduced a schedule_creator, which allows the input file to be load, verified and even manipulated - if needed. The simple standard schedule_creator takes a regular json file, but others could be supported with an overridden schedule_creator_*)
• Added source file information in the configuration file for each of the existing creators
• Make Incofer use the standard schedule_creator; make Fenix rely on the schedule_source information of the config file.

[pantierra]

This one was approved by the CI :ok_hand: Should be generally in a good shape (I hope). It completes the osm2gtfs nicely, because now people don't have to look for the creator related schedule source files. They are now automatically getting downloaded and saved locally. Would be happy to see this getting in. Thanks!

[nlehuby]

I think the schedule_source should be optional (instead of mandatory, with a value of None to be ignored). This would be more coherent with the other optional parameters (start_date). What do you think ?

[pantierra]

For me, the fundamental idea of osm2gtfs is to combine geo-information from OSM with time information (schedule) to produce a GTFS. Not having a schedule to feed in and to rely on hard coded time information in the trip creator (like currently implemented for Accra) is in my opinion a very edge case, and should not be encouraged by making schedule_source optional. However this edge case has been taken into consideration in this pull request and is supported, by opting-in explicitly and specifying None.

[pantierra]

Some information for potential reviewers: Check the two different commits in this PR. They are reflecting two different changes and are probably easier to review first separately:

• Rename schedule to feed, as a result of the conversation in #80
• Make source configurable and downloadable

[pantierra]

Thanks for the review, @grote ❤️

[pantierra]

Because of other changes, this discussion and question got collapsed. So, I'm posting it here again. It is about the question about the issue risen by @nlehuby in one of the first comments - whether schedule_source should be optional.

I proposed a compromise:

• We make schedule_source optional.
• The standard schedule_creator (not the configuration class) throws an error, if there is no file specified.

This would basically follow your opinion, but (at least for the standard creator) do the same checks, as it does currently.

@grote answered and asked:

Sounds like a good compromise to me. @nlehuby?

[pantierra]

This pull request is now at the stage to be tested again - all feedback has been incorporated. The schedule_source is currently implemented as proposed: It is optional in the config file, but the standard schedule_creator checks for a respective json file. For Accra the schedule_creator has been overridden to not process any source.

(in case @nlehuby disagreed, the implementation could be adjusted easily)

[pantierra]

Thanks, @grote! I tested with Accra, but you are right, I didn't run the tests. There were some minor issues with the tests, that I fixed now.

[grote]

When running this branch, I still get:

Traceback (most recent call last):
  File "/home/user/.local/bin/osm2gtfs", line 11, in <module>
    load_entry_point('osm2gtfs', 'console_scripts', 'osm2gtfs')()
  File "/home/user/vcs/osm2gtfs/osm2gtfs/osm2gtfs.py", line 72, in main
    stops_creator.add_stops_to_feed(feed, data)
  File "/home/user/vcs/osm2gtfs/osm2gtfs/creators/stops_creator.py", line 35, in add_stops_to_feed
    self.add_stop(feed, elem)
  File "/home/user/vcs/osm2gtfs/osm2gtfs/creators/stops_creator.py", line 49, in add_stop
    stop_dict["stop_id"] = str(stop.osm_id)
AttributeError: 'Stop' object has no attribute 'osm_id'

Also, there's an unused import lying around still.

[grote]

A tip for testing with fenix: If you hit overpass API, disable the stop name search. This is mostly not necessary for testing.

[pantierra]

Oh, yes, osm_id is already from #96, fixed and tested with Fenix.

[grote]

I can confirm that Fenix works now (although the feed looks way smaller than list time I tried it, need to look into that some day). The Accra tests also pass.

However, Incofer from @jamescr crashes. I think before merging in more PRs, we should first add more tests. Otherwise, we are going to screw things up I'm afraid. Also, manually testing these things is too much work for my taste.

[pantierra]

I think before merging in more PRs, we should first add more tests.

Not nice, to draw this line now :cry: (Incofer fails on master as it does on this one)

[grote]

(Incofer fails on master as it does on this one)

Oh, I didn't know that. I indeed get the same error message on master.

So for me this PR is good to go in.

Not nice, to draw this line now 😢

I am not drawing a line, but merely suggest that we should maybe focus our energies on adding more tests first before merging even more complex PRs. This one already had some issue we only spotted with code review and manual testing. The others will most likely be even more disruptive.

Not all creator maintainers have always time available to review PRs on short notice to make sure their creators are unaffected. Having more tests would reduce the burden on everybody, because we could rely on the CI to tell us whether something broke or not.

[pantierra]

Yes, I also think, the better and more test coverage we have the better we can maintain this growing script! Probably we should consider to require creators with respective tests, as Accra wonderfully did, from all network implementers.

I suggest to create a separate issue to discuss this with all implementers(?)/maintainers(?)/sponsors(?) of the network creators.

[grote]

We have #75 for that. I just increased its priority.

I am just leaving this one open for a bit longer in case one of the others wants to review as well.

[pantierra]

Thanks for the review @ialokim!

[pantierra]

Rebased after the latest pull request which has been accepted: #93.

[pantierra]

All green - all good. Any chance to get it in, anytime soon?

As a side note: Mainly because of the renaming request (from #80), which is implemented in this PR - not having it inside osm2gtfs causes currently significant extra work to keep this in-sync with the other enhancements we are heavily working on to get Managua and Esteli ready.

[grote]

For me it is fine, just waiting a bit with merging to give others a chance to review.

You should be able to rebase other work on top of this branch in the meantime.

[pantierra]

You should be able to rebase other work on top of this branch in the meantime.

Yes, and what I'm trying to express is that it hurts (last sentences of the article):

Don’t rebase branches you have shared with another developer. Do otherwise at your own peril - yes, it will hurt.

[grote]

The same kind of advise applies to force pushing. There's not really a difference between force pushing to this branch and rebasing another branch on this one. Both is easier when you are the only person working on these branches though.

[grote]

Thanks for the review @prhod!

[pantierra]

Wuhuu! Thanks all!