-
Notifications
You must be signed in to change notification settings - Fork 120
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
Update public_transport funk #634
Conversation
LGTM |
@@ -6,9 +6,9 @@ highway,public_transport,railway,route,service,tags->rail,tags->site,min_zoom,ki | |||
*,*,*,funicular;monorail,*,*,*,${12},${route} | |||
*,*,halt;stop;tram_stop,*,*,*,*,${13},${railway} | |||
platform,*,*,*,*,*,*,${15},platform | |||
*,platform,*,*,*,yes,*,${15},platform | |||
*,platform,*,*,*,yes,*,${17},platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the kind here be bus_stop
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch, fixed in 08b3202.
I think the changes to POIs would make it turn something that was plain Also, tests would be good for figuring out what the effect of these would be, i.e: including one for the various different common cases. |
*,*,*,light_rail;tram,*,*,*,*,${9},${route} | ||
*,*,*,funicular;monorail,*,*,*,*,${12},${route} | ||
*,*,halt;stop;tram_stop,*,*,*,*,*,${13},${railway} | ||
platform,*,*,*,*,*,*,*,${15},platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't highway=platform
also be a bus_stop
at z >= 17
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 01cce9a.
@zerebubuth I added a few tests, do you think it looks good enough to merge now? |
Looking good. Let's get this on dev. |
I'll merge this in, we can make any adjustments afterwards if need be. |
Connects to #469
@nvkelso, @zerebubuth could you review please?