Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved finding of names for unnamed stops - #39 #43

Merged
merged 1 commit into from
Nov 19, 2016
Merged

Improved finding of names for unnamed stops - #39 #43

merged 1 commit into from
Nov 19, 2016

Conversation

pantierra
Copy link
Contributor

Related to issue #39.

  • Moved finding of names for unnamed stops logic from FenixStopCreator into OsmHelper
  • Setting flag to search for unnamed stops in config (name_auto)
  • Introduced name for unnamed stops in config (name_without)
  • Moved add_shape() from Route to _generate_shape() in OsmHelper
  • Moved is_valid_stop_candidate() from Stop to _is_valid_stop_candidate() in OsmHelper

@pantierra pantierra changed the title Improved finding of names for unnamed stops Improved finding of names for unnamed stops - #39 Nov 12, 2016
Copy link
Owner

@grote grote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, great work! :)

I will go ahead and merge this. The stop name issues I mentioned can be fixed later.

@@ -314,7 +338,7 @@ def _build_stop(self, stop, stop_type):

# Make sure name is not empty
if 'name' not in stop.tags:
stop.tags['name'] = Stop.NO_NAME
stop.tags['name'] = "[" + self.stop_no_name + "]"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use the square brackets here? When you don't search for stop names and there's stops without name, there will be square brackets in the name. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is! Because I don't want to hit the nearest search if it was unsuccessful once. This is the same behaviour as before.

When creating the stops from the first query the ones without name get something like [No name], then it tries to guess their name, in case it was not possible to find one the name will be No name (without brackets) and next time it would not search (and query overpass api) again.

rv = Route(route_variant.id, fr, to, stops, rm, ref, name)
rv.add_shape(route_variant, query_result_set)
shape = self._generate_shape(route_variant, query_result_set)
rv = Route(route_variant.id, fr, to, stops, rm, ref, name, shape)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice the shape now gets supplied at construction time!

for stop in self.stops.values():

# If there is no name, query one intelligently from OSM
if stop.name == "[" + self.stop_no_name + "]":
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is how you identify the stops that haven't got a name. Maybe it is better use None here and only change all that are still none None after this method ran (or didn't ran) to the variable from the config file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I like the proposal of using None. It just left it as it was before, in order to keep pull requests small. Here is a new issue for this: #47

@grote grote merged commit d296a12 into grote:master Nov 19, 2016
@pantierra pantierra mentioned this pull request Nov 19, 2016
@pantierra pantierra deleted the issue-39 branch November 19, 2016 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants