Data structure #89
Data structure #89
Conversation
|
@xamanu thanks for this! Do you know why the CI didn't run for this MR? Do you mind rebasing this on |
|
Will rebase and check. |
osm2gtfs/core/osm_connector.py
Outdated
| line.add_itinerary(itinerary) | ||
| except ValueError: | ||
| print('Itinerary ID does not match line ID. Please fix in OSM.') | ||
| print(line.osm_url) |
There was a problem hiding this comment.
These kind of errors should be printed to stderr like elsewhere.
osm2gtfs/core/osm_connector.py
Outdated
|
|
||
| Returns a initiated RouteVariant object from raw data | ||
|
|
||
| colour = "FFFFFF" |
There was a problem hiding this comment.
I think we should be in line with transitfeed's way of writing color here (and below).
There was a problem hiding this comment.
British... American... now it's time to chose 😄 I also tend to use the American over the British as in most technical environments it is the quasi standard. I would go for color, what do you think, @AltNico?
osm2gtfs/core/osm_connector.py
Outdated
|
|
||
| colour = "FFFFFF" | ||
| if "colour" in route_master.tags: | ||
| colour = OsmConnector.get_hex_code_for_color(route_master.tags['colour']) |
There was a problem hiding this comment.
Do the tags really use colour instead of color? Is this defined somewhere?
There was a problem hiding this comment.
Yes, in OpenStreetMap usually British English is used: https://wiki.openstreetmap.org/wiki/Key:colour
There was a problem hiding this comment.
What about supporting both? 😄
osm2gtfs/core/osm_connector.py
Outdated
| self.stops["relation/" + str(relation.id) | ||
| ] = self._build_stop_area(relation) | ||
| except RuntimeError: | ||
| print('Cannot add stop area', relation.id) |
There was a problem hiding this comment.
We should not catch RuntimeErrors. Please use a different exception for this. You can even make your own.
osm2gtfs/core/osm_connector.py
Outdated
| @@ -598,7 +646,7 @@ def _find_best_name_for_unnamed_stop(self, stop): | |||
| winner_distance = sys.maxint | |||
| for candidate in candidates: | |||
| if isinstance(candidate, overpy.Way): | |||
| lat, lon = Stop.get_center_of_nodes( | |||
| lat, lon = OsmConnector.get_center_of_nodes( | |||
There was a problem hiding this comment.
We do you need to prepend method calls with OsmConnector when you are in OsmConnector?
osm2gtfs/core/stops.py
Outdated
| str(osm_type) + "/" + str(osm_id)) | ||
|
|
||
|
|
||
| class StopArea(object): |
There was a problem hiding this comment.
This doesn't need to be annotated with @attr.s?
| class Stop(object): | ||
|
|
||
| osm_id = attr.ib() | ||
| osm_type = attr.ib() |
There was a problem hiding this comment.
Why do we need an osm_type? What values can this have?
There was a problem hiding this comment.
It can be either node or way. Depending on the type different behaviours might be necessary. E.g for a way you may want to find the center. And we concretely need it for constructing the url to the object here.
There was a problem hiding this comment.
Another reason is also related to #94: osm_id is used for different id spaces, depending on osm_type. Both together can identify an object in OSM, not only osm_id. For the id of the GTFS it is recommended to combine those two in any way.
| @@ -78,6 +79,7 @@ def _get_itinerary_operation(self, itinerary, | |||
| operations.append("sunday") | |||
| return operations | |||
|
|
|||
| # pylint: disable=arguments-differ | |||
| def _create_service_period(self, schedule, operation): | |||
osm2gtfs/creators/routes_creator.py
Outdated
| route_url # From Line | ||
| route_color # From Line | ||
| route_text_color # From Line | ||
| """ |
There was a problem hiding this comment.
What do these comments mean? What do they relate to?
There was a problem hiding this comment.
They were just to not forget important values 😄 Can be taken out.
|
Thanks a lot for the review, @grote! I just fixed the noted issues. Please note: When testing, make sure you clean all your cached data. |
|
Please check your project settings for Integration & Services and make sure Travis CI is listed there and enabled. |
osm2gtfs/core/osm_connector.py
Outdated
|
|
||
| # Cache data | ||
| Cache.write_data('stops-' + self.selector, self.stops) | ||
| Cache.write_data('stops-' + self.selector, self.routes) |
osm2gtfs/core/osm_connector.py
Outdated
| sys.stderr.write("http://osm.org/node/" + | ||
| str(member.ref) + "\n") | ||
| if len(stop_members) < 1: | ||
| raise RuntimeError('Cannot build stop area with no members') |
There was a problem hiding this comment.
Looks like you are still raising a RuntimeError here.
|
Thanks, now CI pipeline works. Still a bit to fix, but we are close to green 😄 Will comment then. |
|
It seems like the last error raised here by pylint through the Travis CI are part of attr.s and can not be fixed: python-attrs/attrs#215 |
|
Closed in favour of this rebased version: #96 |
This is a new pull request for the revised data structure #30 with a lot of wonderful work by @AltNico.
Please help with your review. Thanks! 😄