Refactor OsmHelper about osm2gtfs HOT 6 CLOSED

I visualized it in a diagram:

from osm2gtfs.

Looks nice!

Where are the methods to refresh a route (and maybe even a stop)? Is this cache_refresh? If so, maybe instead of passing a name, have dedicated methods for each type.

I was passing the routes into get_stops() and sometimes chaining these calls, because there's no point getting a stop if it is not part of a route relation, right? Also, you when you refresh a route, this might add new stops that you didn't have before.

The same applies when you refresh a route master, you might get a new route variant and new stops.

from osm2gtfs.

Refresh routes, stops and single route would happen through cache_refesh with an argument stating what to refresh:

• cache_refresh('routes')
• cache_refresh('route', '255')
• cache_refresh('stops')

The function cache_refresh contains a if/elseif statement select what functions to run for refresh. Personally I prefer to have one wrapper function than three single-line functions.

I think it's better to handle stops independently of routes. I can think about several reasons for this:

• We can optimize Overpass queries to not query again and again for each route's stops.
• We can do validation and e.g. print a warning, when stops are not part of any route, or likewise if a route has a stop that is not properly tagged.
• transitfeed handles stops independently from routes and we can also run logic better in encapsulated StopsCreators and RoutesCreators.

For the case when a route refresh comes up with new stops that are not cached already, I introduced the query_stop(osm_id) function to lazy load these stops.

I generally would like to handle all OSM specific things inside OsmHelper. This means, applied to routes, that from outside (of OsmHelper) there only exist routes (like in GTFS). All RouteMaster and RouteVariant stuff is handled inside the helper. When you refresh a route OsmHelper checks the master relation and route variants for it to be refreshed.

from osm2gtfs.

Alright, sounds good. The cache refresh method is a matter of taste. If we might want to refresh different things in the future, I would agree with the parameter approach, but there will never be more than stops and routes. The arbitrary parameters you need to to error handling and offload to independent methods anyway. So I'd say using dedicated refresh methods is cleaner.

from osm2gtfs.

Yes, indeed, it's a matter of taste. I don't really care. Let's look how it looks like when we implement it. The only addition to refresh I can think of right now, is what you said above: refresh a single stop.

from osm2gtfs.

Refactored with the latest commit. Documenting here the changes and closing the issue.

• Renamed to OsmConnector
• Moved all caching to a Cache class
• Added comments to program code

Implemented the following structure:

OsmConnector

- config
- routes {route_id, RouteMaster}
- stops {osm_id, Stop}
-------
+ get_routes(): Dic of RouteMaster
+ get_stops(): Dic of Stop

- _build_route_master(): RouteMaster
- _build_route_variant(): Route
- _build_route_stop(): Stop


- _query_routes(): Dic of RouteMaster
- _query_stops(): List of Stop

from osm2gtfs.