Isochrone api #621

Udumft · 2021-10-25T14:30:41Z

Resolves #296

This PR adds Isochrone object to access co-named API. It's structure and tooling entities are similar to Directions.

Sources/MapboxDirections/IsochroneCredentials.swift

1ec5 · 2021-10-25T17:10:22Z

Sources/MapboxDirections/IsochroneError.swift

+            case (200, "NoSegment"):
+                self = .unableToLocate
+            case (404, "ProfileNotFound"):
+                self = .profileNotFound
+            case (422, "InvalidInput"):
+                self = .invalidInput(message: message)
+            case (429, _):
+                self = .rateLimited(rateLimitInterval: response.rateLimitInterval, rateLimit: response.rateLimit, resetTime: response.rateLimitResetTime)


This table lists a few more codes that should get dedicated cases, but they sound like codes that would be returned by other navigation service APIs too. Some of the codes may come with structured properties alongside the message, similar to the 429 error, which would be nice to parse into associated values.

Do any codes conflict with Directions API error codes? Could they conflict in the future?

There are some troubles with this table. In the code I've listed all errors I managed to reproduce.

Sources/MapboxDirections/IsochroneOptions.swift

1ec5 · 2021-10-25T17:16:03Z

Sources/MapboxDirections/IsochroneProfileIdentifier.swift

+/**
+ Options determining the primary mode of transportation for the contours.
+ */
+public struct IsochroneProfileIdentifier: Codable, RawRepresentable {


This should probably be consolidated with DirectionsProfileIdentifier unless there’s a possibility that some profile will intentionally be limited to one service or another.

There is a difference already: Isochrone does not support driving-traffic profile.

I think a limitation like that wouldn’t keep us from reusing DirectionsProfileIdentifier. We can just document the fact that the automobileAvoidingTraffic constant can’t be used with the Isochrone class. What would be a problem is if the Isochrone API’s mapbox/driving-traffic profile behaved like the Directions API’s mapbox/driving profile or vice versa. Besides, these are only constants in an extensible enumeration: the developer is always able to specify an invalid profile identifier like mabpox/unicycling.

Do you think it would be wise to rename DirectionsProfileIdentifier to something more generic like ServiceProfileIdentifier since it is used in multiple services and we are going to document it's effect on each one in code docs? That would be a breaking change.
I would like to avoid using exactly Directions<Something> for Isochrone, plus mentioning Isochrones in it's automobileAvoidingTraffic would also look strange.

Do you think it would be wise to rename DirectionsProfileIdentifier to something more generic like ServiceProfileIdentifier since it is used in multiple services and we are going to document it's effect on each one in code docs? That would be a breaking change.

I think it’s totally reasonable. ProfileIdentifier should be sufficient; I don’t think it’s very likely to introduce a lot of conflicts with other libraries. We can avoid a breaking change by leaving in a type alias that’s marked as deprecated.

1ec5 · 2021-10-26T21:42:38Z

Sources/MapboxDirections/IsochroneProfileIdentifier.swift

+/**
+ Options determining the primary mode of transportation for the contours.
+ */
+public struct IsochroneProfileIdentifier: Codable, RawRepresentable {


I think a limitation like that wouldn’t keep us from reusing DirectionsProfileIdentifier. We can just document the fact that the automobileAvoidingTraffic constant can’t be used with the Isochrone class. What would be a problem is if the Isochrone API’s mapbox/driving-traffic profile behaved like the Directions API’s mapbox/driving profile or vice versa. Besides, these are only constants in an extensible enumeration: the developer is always able to specify an invalid profile identifier like mabpox/unicycling.

Sources/MapboxDirections/Isochrone.swift

Sources/MapboxDirections/IsochroneOptions.swift

1ec5 · 2021-10-26T22:46:50Z

CHANGELOG.md

@@ -6,6 +6,7 @@
 * Added `RouteOptions.initialManeuverAvoidanceRadius` property to control avoidance of dangerous maneuvers when requesting a route from the Directions API. ([#609](https://github.com/mapbox/mapbox-directions-swift/pull/609))
 * Added `RoadClasses.unpaved` property that enables the resulting routes to avoid unpaved roads. ([#620](https://github.com/mapbox/mapbox-directions-swift/pull/620))
 * Added `RoadClasses.cashOnlyToll` property that enables the resulting routes to avoid toll roads that only accept cash payment. ([#620](https://github.com/mapbox/mapbox-directions-swift/pull/620))
+* Added `Ischrone` API wrapper. The [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) computes areas that are reachable within a specified amount of time from a location, and returns the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))


Let’s add a basic usage example to the readme. The example can assume the presence of the map SDK or MapboxStatic.swift. This can be tail work.

I think an Example would be even better. Could we add it to navigation-ios-examples? It's not related to navigation though, but looks like we don't have a dedicated place for Directions examples at all?

Yes, mapbox-navigation-ios-examples is a good place to showcase what MapboxDirections can do on its own. We should still nod to the existence of Isochrone API support in the readme though.

Added example usage to README

Incidentally, it never occurred to me that it’s the Isochrone API, singular. We had made a big push to name all the server APIs in the plural, but the navigation service APIs must’ve escaped notice apart from the Directions API.

Suggested change

* Added `Ischrone` API wrapper. The [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) computes areas that are reachable within a specified amount of time from a location, and returns the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

* Added `Isochrones`, which connects to the [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) to compute areas that are reachable within a specified amount of time from a location and return the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

* Renamed `DirectionsCredentials` and `DirectionsProfileIdentifier` to `Credentials` and `ProfileIdentifier`, respectively. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

The class is currently called Isochrones, plural.

Suggested change

* Added `Ischrone` API wrapper. The [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) computes areas that are reachable within a specified amount of time from a location, and returns the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

* Added `Isochrones`, which connects to the [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) to compute areas that are reachable within a specified amount of time from a location, returning the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

* Renamed `DirectionsCredentials` and `DirectionsProfileIdentifier` to `Credentials` and `ProfileIdentifier`, respectively. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

Sources/MapboxDirections/IsochroneOptions.swift

1ec5 · 2021-10-27T20:07:53Z

Sources/MapboxDirections/IsochroneOptions.swift

+            /**
+             Contour bound definition value and contour color.
+             */
+            public typealias ValueAndColor = (value: Value, color: Color)


My suggestion in #621 (comment) didn’t consider the possibility of specifying the color on none of the contours (which the API turns into a rainbow by default). The type system gets a bit complicated as a result. If additional per-contour properties get added in the future, such as opacity, we could be forced to make this enumeration more complicated. A struct also gives us more flexibility to maintain backwards compatibility.

What if we stick to something similar to the design in #621 (comment) but make color optional? On the client-side, we can fill in a default color at request time if colors are specified for some contours but not others. This wouldn’t violate the API contract, because the API doesn’t allow leaving some colors unspecified in the first place. But if it does in the future, it would be trivial to remove the fallback.

Filling colors gap automatically seems error prone. We cannot track if API will change the default color scheme, we'll also be basically guessing which default color to substitute in each particular gap.
Since we are making an API wrapper, it should make usage convenient but not alter the original behavior.

It’s not so much for the developer’s convenience but for us to ensure the client-side API’s flexibility:

If the Isochrone API later introduces the ability to leave one of the colors unspecified, then the fallbacks would become extraneous, but everything the developer could previously do would remain possible, and it wouldn’t be possible to specify an invalid set of contours. On the other hand, with these tuples, we’d have to introduce a ValueAndOptionalColor tuple and corresponding enumeration case.

If the API adds a contours_opacity parameter, then we’d need to introduce new enumeration cases and tuples for ValueAndColorAndOpacity and ValueAndOpacity, and it could be combinatorial explosion after that. On the other hand, with a single struct, we can add additional optional properties without any fuss.

We could fill in black as a reasonable default color. Rainbow colors would still be possible if none of the colors is specified.

Struct approach turned out 1 word shorter during initialization. I decided that using black as a fallback color might be too much, so I chose a bit more neutral 50% gray. :)

Sources/MapboxDirections/IsochroneOptions.swift

Tests/MapboxDirectionsTests/IsochroneTests.swift

README.md

1ec5

Some additional documentation-related nits, but I think the big question is how to structure the type system to dissuade the user from making invalid requests while being forward-compatible: #621 (comment).

README.md

1ec5 · 2021-10-28T23:41:00Z

README.md

+        let encoder = JSONEncoder()
+        let data = try! encoder.encode(response)
+        let geoJSONString = String(data: data, encoding: .utf8)!
+        let geoJSONOverlay = GeoJSON(objectString: geoJSONString)


mapbox/MapboxStatic.swift#112 tracks streamlining these steps by adding Turf support to the library.

1ec5 · 2021-10-28T23:44:05Z

CHANGELOG.md

@@ -6,6 +6,7 @@
 * Added `RouteOptions.initialManeuverAvoidanceRadius` property to control avoidance of dangerous maneuvers when requesting a route from the Directions API. ([#609](https://github.com/mapbox/mapbox-directions-swift/pull/609))
 * Added `RoadClasses.unpaved` property that enables the resulting routes to avoid unpaved roads. ([#620](https://github.com/mapbox/mapbox-directions-swift/pull/620))
 * Added `RoadClasses.cashOnlyToll` property that enables the resulting routes to avoid toll roads that only accept cash payment. ([#620](https://github.com/mapbox/mapbox-directions-swift/pull/620))
+* Added `Ischrone` API wrapper. The [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) computes areas that are reachable within a specified amount of time from a location, and returns the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))


Incidentally, it never occurred to me that it’s the Isochrone API, singular. We had made a big push to name all the server APIs in the plural, but the navigation service APIs must’ve escaped notice apart from the Directions API.

Suggested change

* Added `Ischrone` API wrapper. The [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) computes areas that are reachable within a specified amount of time from a location, and returns the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

* Added `Isochrones`, which connects to the [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) to compute areas that are reachable within a specified amount of time from a location and return the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

* Renamed `DirectionsCredentials` and `DirectionsProfileIdentifier` to `Credentials` and `ProfileIdentifier`, respectively. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

Sources/MapboxDirections/IsochroneOptions.swift

Sources/MapboxDirections/Isochrones.swift

…it tests added.

…fier from enum to struct

… typealias; refactored IsochroneOptios.Contour and Color entities; extended tests to verify contours_minutes coding

Squashed commits: [91ac378] vk-296-isochrone-api: linux tests rearranged [fd9d7dd] vk-296-isochrone-api: resolved OHTTPStubs Linux usage

…worked contours definition.

…finition

…for NSColor, refined contour query serialization code.

…ProfileIdentifier to reuse with Isohrones. Tests updated

… struct. Tests and examples updated

1ec5 · 2021-11-11T02:41:53Z

CHANGELOG.md

@@ -6,6 +6,7 @@
 * Added `RouteOptions.initialManeuverAvoidanceRadius` property to control avoidance of dangerous maneuvers when requesting a route from the Directions API. ([#609](https://github.com/mapbox/mapbox-directions-swift/pull/609))
 * Added `RoadClasses.unpaved` property that enables the resulting routes to avoid unpaved roads. ([#620](https://github.com/mapbox/mapbox-directions-swift/pull/620))
 * Added `RoadClasses.cashOnlyToll` property that enables the resulting routes to avoid toll roads that only accept cash payment. ([#620](https://github.com/mapbox/mapbox-directions-swift/pull/620))
+* Added `Ischrone` API wrapper. The [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) computes areas that are reachable within a specified amount of time from a location, and returns the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))


The class is currently called Isochrones, plural.

Suggested change

* Added `Ischrone` API wrapper. The [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) computes areas that are reachable within a specified amount of time from a location, and returns the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

* Added `Isochrones`, which connects to the [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) to compute areas that are reachable within a specified amount of time from a location, returning the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

* Renamed `DirectionsCredentials` and `DirectionsProfileIdentifier` to `Credentials` and `ProfileIdentifier`, respectively. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

Sources/MapboxDirections/DirectionsProfileIdentifier.swift

1ec5 · 2021-11-11T02:43:18Z

Sources/MapboxDirections/DirectionsOptions.swift

@@ -291,7 +291,7 @@ open class DirectionsOptions: Codable {
    // MARK: Getting the Request URL

    /**
-     An array of URL query items to include in an HTTP request.
+     The path of the request URL, specifying service name, version and profile.


Thanks for catching this!

1ec5 · 2021-11-11T02:45:14Z

Sources/MapboxDirections/IsochroneOptions.swift

+    /**
+     Contours GeoJSON format.
+     */
+    public enum ContourFormat {


Do we need to nest this type under IsochroneOptions? Application code might end up needing to write IsochroneOptions.ContourFormat in some cases, but ContourFormat on its own would probably not risk future conflicts with other API wrappers in this library.

I've nested this one since there's no practical use of this enum other than inside IsochroneOptions. Cases when fully qualified name would be required for typing are pretty rare (Linux only?) since there is only 1 place (property) where it is used and it's strictly typed.

Sources/MapboxDirections/IsochroneOptions.swift

1ec5 · 2021-11-11T02:50:50Z

Sources/MapboxDirections/Isochrones.swift

+                        return
+                    }
+
+                    guard (disposition.code == nil && disposition.message == nil) || disposition.code == "Ok" else {


There’s a bit of redundancy between this method and some of the Directions methods. As tail work, we could try factoring out some common tasks so it’s easier and less error-prone to spin up support for a new API.

Sources/MapboxDirections/DirectionsCredentials.swift

…s and DirectionsProfileIdentifier alieases to original implementations; code docs corrected.

Udumft added improvement Improvement for an existing feature. platform parity labels Oct 25, 2021

Udumft added this to the v2.1-beta milestone Oct 25, 2021

Udumft self-assigned this Oct 25, 2021

1ec5 reviewed Oct 25, 2021

View reviewed changes

Udumft force-pushed the vk/296-isochrone-api branch 2 times, most recently from 91ac378 to 34ffe06 Compare October 26, 2021 10:00

Udumft marked this pull request as ready for review October 26, 2021 10:20

Udumft requested a review from a team October 26, 2021 10:20

1ec5 reviewed Oct 26, 2021

View reviewed changes

1ec5 reviewed Oct 27, 2021

View reviewed changes

jill-cardamon reviewed Oct 28, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Udumft requested review from 1ec5 and jill-cardamon October 28, 2021 14:14

1ec5 reviewed Oct 28, 2021

View reviewed changes

Udumft requested a review from 1ec5 October 29, 2021 12:36

Udumft force-pushed the vk/296-isochrone-api branch from 2cab7b1 to ebc4ed1 Compare November 1, 2021 08:07

jill-cardamon reviewed Nov 5, 2021

View reviewed changes

Sources/MapboxDirections/IsochroneOptions.swift Outdated Show resolved Hide resolved

1ec5 added the isochrones Isochrone API label Nov 5, 2021

jill-cardamon reviewed Nov 7, 2021

View reviewed changes

Sources/MapboxDirections/Isochrones.swift Outdated Show resolved Hide resolved

Udumft added 10 commits November 8, 2021 18:34

vk-296-isochrone-api: added Isochrone class with related entities. Un…

efa922e

…it tests added.

vk-296-isochrone-api: added code docs, changed IsochroneProfileIdenti…

d3cae7a

…fier from enum to struct

vk-296-isochrone-api: CHANGELOG updated

05ba71d

vk-296-isochrone-api: Linux build fix

a36b69b

vk-296-isochrone-api: replace IsochroneCredentials definition to be a…

b14e55d

… typealias; refactored IsochroneOptios.Contour and Color entities; extended tests to verify contours_minutes coding

vk-296-isochrone-api: test data typo fix (+2 squashed commits)

b36d42b

Squashed commits: [91ac378] vk-296-isochrone-api: linux tests rearranged [fd9d7dd] vk-296-isochrone-api: resolved OHTTPStubs Linux usage

vk-296-isochrone-api: linux build color fix

c2a31ff

vk-296-isochrone-api: refactoed IsochroneOptions: renamed members, re…

5060f55

…worked contours definition.

vk-296-isochrone-api: incorporated Measurement for contours bounds de…

66fe022

…finition

vk-296-isochrone-api: linux compilation fixed.

104a9b5

Udumft added 8 commits November 8, 2021 18:35

vk-296-isochrone-api: corrected naming, added color space converting …

e658370

…for NSColor, refined contour query serialization code.

vk-296-isochrone-api: added README example; fixed query encoding bug

9420de4

vk-296-isochrone-api: deprecated DirectionsCredentials and Directions…

d0584be

…ProfileIdentifier to reuse with Isohrones. Tests updated

vk-296-isochrone-api: respored xcodeproj file references

630bf2e

vk-296-isochrone-api: test compilation issue fixed

4548f2c

vk-296-isochrone-api: replaced ContourDefinition enum with Definition…

f979052

… struct. Tests and examples updated

vk-296-isochrone-api: linux build fix

96b852d

vk-296-isochrones-api: code docs corrected.

c196293

Udumft force-pushed the vk/296-isochrone-api branch from e27b811 to c196293 Compare November 8, 2021 15:36

Udumft requested a review from jill-cardamon November 8, 2021 15:48

1ec5 approved these changes Nov 11, 2021

View reviewed changes

1ec5 modified the milestones: v2.1-beta, v2.1-rc Nov 11, 2021

vk-296-isochrone-api: CHANGELOG corrected; moved DirectionsCredential…

4b4ddf5

…s and DirectionsProfileIdentifier alieases to original implementations; code docs corrected.

Udumft merged commit f46b02e into main Nov 11, 2021

Udumft deleted the vk/296-isochrone-api branch November 11, 2021 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Isochrone api #621

Isochrone api #621

Udumft commented Oct 25, 2021

1ec5 Oct 25, 2021

Udumft Oct 26, 2021

1ec5 Oct 25, 2021

Udumft Oct 26, 2021

1ec5 Oct 26, 2021

Udumft Oct 27, 2021

1ec5 Oct 27, 2021 •

edited

Loading

1ec5 Oct 26, 2021

1ec5 Oct 26, 2021 •

edited

Loading

Udumft Oct 27, 2021

1ec5 Oct 27, 2021

Udumft Oct 28, 2021

1ec5 Oct 28, 2021

1ec5 Nov 11, 2021

1ec5 Oct 27, 2021

Udumft Oct 28, 2021

1ec5 Oct 28, 2021 •

edited

Loading

Udumft Oct 29, 2021

1ec5 left a comment

1ec5 Oct 28, 2021

1ec5 Oct 28, 2021

1ec5 Nov 11, 2021

1ec5 Nov 11, 2021

1ec5 Nov 11, 2021

Udumft Nov 11, 2021

1ec5 Nov 11, 2021

	* Added `Ischrone` API wrapper. The [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) computes areas that are reachable within a specified amount of time from a location, and returns the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))
	* Added `Isochrones`, which connects to the [Mapbox Isochrone API](https://docs.mapbox.com/api/navigation/isochrone/) to compute areas that are reachable within a specified amount of time from a location and return the reachable regions as contours of polygons or lines that you can display on a map. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))
	* Renamed `DirectionsCredentials` and `DirectionsProfileIdentifier` to `Credentials` and `ProfileIdentifier`, respectively. ([#621](https://github.com/mapbox/mapbox-directions-swift/pull/621))

Isochrone api #621

Isochrone api #621

Conversation

Udumft commented Oct 25, 2021

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 Oct 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Oct 26, 2021 • edited Loading

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 Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 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 Oct 27, 2021 •

edited

Loading

1ec5 Oct 26, 2021 •

edited

Loading

1ec5 Oct 28, 2021 •

edited

Loading