Structured road names, codes, destinations #91

1ec5 · 2016-11-09T06:06:30Z

Changed RouteStep’s name and destinations properties into arrays, so that client code doesn’t have to do any splitting or parsing on its own. Removed redundant refs from the name property. Added a codes property that contains the route numbers. Though a Route struct type is used internally during parsing, all the new properties live directly on RouteStep for easier Objective-C bridging.

Also updated test fixtures and tests.

Fixes #67.

/cc @bsudekum @freenerd

freenerd

Looking good from my side, can't wait to integrate with instructions 😄

freenerd · 2016-11-10T11:59:33Z

MapboxDirections/MBRouteStep.swift

+    init(name: String?, ref: String?, destination: String?) {
+        var codes: [String]?
+        if let names = name, let ref = ref ?? destination {
+            // OSRM v5.5 encodes the ref separately from the name but redundantly includes the ref in the name for backwards compatibility. Remove the ref from the name.


This is not a feature of OSRM but of Mapbox Directions backwards-compatiblity

freenerd · 2016-11-10T12:00:15Z

MapboxDirections/MBRouteStep.swift

+            }
+            codes = ref.tagValuesSeparatedByString(";")
+        } else if let names = name, let codesRange = names.rangeOfString("\\(.+?\\)$", options: .RegularExpressionSearch, range: names.startIndex..<names.endIndex) {
+            // OSRM v5.0 encodes the ref inside a parenthetical. Remove the ref from the name.


I think we are good dropping support for older OSRM versions

Let’s tackle removing Directions API v4 support in a separate PR.

Changed RouteStep’s name and destinations properties into arrays, so that client code doesn’t have to do any splitting or parsing on its own. Removed redundant refs from the name property. Added a codes property that contains the route numbers. Updated test fixtures and tests.

freenerd · 2016-11-15T09:08:12Z

MapboxDirections/MBRouteStep.swift

+
+    init(name: String?, ref: String?, destination: String?) {
+        var codes: [String]?
+        if let names = name, let ref = ref ?? destination {


let ref = ref ?? destination

I think we should not put destination into ref, since these are difference concepts. We also have extra destination handling below, so I think ?? destination can be removed here.

Note that name will never be undefined/nil from OSRM, but an empty string "". So effectively this if only tests for ref being present.

ref is a misnomer here: what I really meant is “whatever is inside the parentheses”, which can be either a ref or a destination, due to munging that either the Directions API or OSRM performs (not sure which).

Fixed and fixed.

freenerd · 2016-11-15T09:08:57Z

MapboxDirections/MBRouteStep.swift

+        } else if let names = name, let codesRange = names.rangeOfString("\\(.+?\\)$", options: .RegularExpressionSearch, range: names.startIndex..<names.endIndex) {
+            // Mapbox Directions API v4 encodes the ref inside a parenthetical. Remove the ref from the name.
+            let parenthetical = names.substringWithRange(codesRange)
+            if names == ref ?? destination {


same as above, remove ?? destination

freenerd · 2016-11-15T09:16:35Z

MapboxDirections/MBRouteStep.swift

+
+        let road = Road(name: name, ref: json["ref"] as? String, destination: json["destinations"] as? String)
+        names = road.names
+        codes = road.codes ?? road.destinationCodes


Why are destinationCodes mixed into codes here? wouldn't it make more sense to create a new property to store them instead?

The idea here was to make it easier for applications to display a route shield for an upcoming ramp based on codes without having to consult a separate property. We’ve since moved away from the notion that MapboxDirections.swift should perform any sort of munging, so I’ll remove this fallback.

I’ve gone back and forth quite a bit about whether destination codes should be conflated with destination or live in a separate property. In OSM, destination:ref usually follows a different format and encodes more information than ref. For example, if I-80 travels over a ramp to go from one motorway to another, the highway=motorway_link way is tagged ref=I 80 but may also be tagged destination:ref=I-80 West. The format is loose enough that I don’t feel confident making destinationCodes look like codes. But I guess it can’t hurt much.

Fixed. destinationCodes is a separate property now.

freenerd · 2016-11-15T09:17:50Z

MapboxDirections/MBRouteStep.swift

+        let road = Road(name: name, ref: json["ref"] as? String, destination: json["destinations"] as? String)
+        names = road.names
+        codes = road.codes ?? road.destinationCodes
+        destinations = road.destinations


I assume these are properties on self.? Either these should be set that way, or below assignments should not have self.= either (like self.intersections = -> intersections =)

I’ll remove self. from self.intersections, since there isn’t a conflict with a local variable named intersections.

freenerd

ref ?? destination continues to confuse me. I think these should stay separated, so they can be handled differently downstream. If you want to make sure that for a certain step we always have the correctly-chosen identifier, that should be handled via helper functions that look over all the data instead during initialization.

freenerd · 2016-11-15T09:20:39Z

MapboxDirections/MBRouteStep.swift

+
+    init(name: String?, ref: String?, destination: String?) {
+        var codes: [String]?
+        if let names = name, let gloss = ref ?? destination {


I don't understand the semantic meaning of gloss and the dictionary did not help me understand. The case that in Mapbox Directions v5 destination is mixed into name can not exist. Likewise it makes no sense to me that codes would ever have the contents of destination in them.

Turns out Directions API v5 used to conflate destination with ref in the parenthetical, but it no longer does so, much to my relief. I’ll gladly remove the fallbacks.

freenerd · 2016-11-15T09:23:36Z

MapboxDirectionsTests/V5Tests.swift

@@ -54,7 +54,7 @@ class V5Tests: XCTestCase {

        XCTAssertNotNil(route)
        XCTAssertNotNil(route!.coordinates)
-        XCTAssertEqual(route!.coordinates!.count, 842)
+        XCTAssertEqual(route!.coordinates!.count, 28_442)


What's the semantics of 28_442? Is this a range or math exponent syntax?

_ groups digits; it’s for human edification only and has no effect on the literal’s meaning. (C++ uses ' for the same purpose.)

freenerd · 2016-11-15T09:24:56Z

MapboxDirectionsTests/V5Tests.swift


-        let lane = intersection.approachLanes?.first
+        let lane = intersection?.approachLanes?.first
        let indications = lane?.indications
        XCTAssertNotNil(indications)
        XCTAssertTrue(indications!.contains(.Left))


it might make sense to add a test for step.names being populated.

Added a test for this case.

freenerd · 2016-11-15T09:28:27Z

MapboxDirections/MBRouteStep.swift

-            let parenthetical = "(\(ref))"
-            if names == ref {
+        if let names = name, let gloss = ref ?? destination {
+            // Mapbox Directions API v5 encodes the ref separately from the name but redundantly includes the ref or destination in the name for backwards compatibility. Remove the ref or destination from the name.


To clarify: This is not true, the direction is never mixed into the name, but only the ref.

Removed an affordance for the destination being placed inside parentheses inside the name field, since the Directions API no longer does that. Separated destinationCodes from destinations and clarified the formats of codes and destinationCodes in OpenStreetMap data. Added tests of names in steps.

1ec5 added the improvement Improvement for an existing feature. label Nov 9, 2016

1ec5 self-assigned this Nov 9, 2016

1ec5 mentioned this pull request Nov 9, 2016

Add rotary and use lane types #93

Merged

1ec5 force-pushed the 1ec5-ref-67 branch from 66023b6 to 186d779 Compare November 10, 2016 06:56

freenerd reviewed Nov 10, 2016

View reviewed changes

1ec5 added 2 commits November 15, 2016 00:43

Clarified comments about API versions

4192690

1ec5 force-pushed the 1ec5-ref-67 branch from 186d779 to 4192690 Compare November 15, 2016 08:45

freenerd reviewed Nov 15, 2016

View reviewed changes

Clarified purpose of ref/destination capturing

09b6300

freenerd reviewed Nov 15, 2016

View reviewed changes

freenerd suggested changes Nov 15, 2016

View reviewed changes

freenerd reviewed Nov 15, 2016

View reviewed changes

1ec5 added 2 commits November 15, 2016 02:02

Step name is required but may be empty

1ff4054

freenerd approved these changes Nov 15, 2016

View reviewed changes

Merge branch 'master' into 1ec5-ref-67

ce32527

1ec5 merged commit d37f7f0 into master Nov 15, 2016

1ec5 deleted the 1ec5-ref-67 branch November 15, 2016 16:54

freenerd mentioned this pull request Nov 16, 2016

Always retain ref if exist #99

Merged

1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Dec 18, 2018

1ec5 added this to the v0.7.0 milestone Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structured road names, codes, destinations #91

Structured road names, codes, destinations #91

1ec5 commented Nov 9, 2016 •

edited

Loading

freenerd left a comment

freenerd Nov 10, 2016

freenerd Nov 10, 2016

1ec5 Nov 15, 2016

freenerd Nov 15, 2016

freenerd Nov 15, 2016

1ec5 Nov 15, 2016

1ec5 Nov 15, 2016

freenerd Nov 15, 2016

freenerd Nov 15, 2016

1ec5 Nov 15, 2016

1ec5 Nov 15, 2016

freenerd Nov 15, 2016

1ec5 Nov 15, 2016

freenerd left a comment

freenerd Nov 15, 2016

1ec5 Nov 15, 2016

freenerd Nov 15, 2016

1ec5 Nov 15, 2016

freenerd Nov 15, 2016

1ec5 Nov 15, 2016

freenerd Nov 15, 2016

Structured road names, codes, destinations #91

Structured road names, codes, destinations #91

Conversation

1ec5 commented Nov 9, 2016 • edited Loading

freenerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freenerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Nov 9, 2016 •

edited

Loading