Adjust naming of offline tile downloading methods #353

akitchen · 2019-02-21T05:23:19Z

Fixes #352

MapboxDirectionsTests/OfflineDirectionsTests.swift

frederoni · 2019-02-21T13:34:00Z

The flaky GeoJSON tests may be caused by unsorted JSON.

akitchen · 2019-02-21T20:37:25Z

The flaky GeoJSON tests may be caused by unsorted JSON.

I ran it again and it passed. Can't seem to reproduce locally.

where is the non-determinisim? there seems to be a timeout in the test, which could indicate the OHHTTPStubs isn't yet bootstrapped properly, or there's a race. I think we've seen this before with NSURLProtocol-based network stubbing in tests.

I don;t think this should block merge of this PR (assuming we're 👍 otherwise) but we do need to get to the bottom of it.

frederoni · 2019-02-21T21:15:07Z

where is the non-determinisim?

I can't find the non-deterministic URL here, but it MapboxStatic.swift, the OHHTTPStub parsed GeoJSON when setting up the router which would lead to undeterministic route/url.

We worked around that in the test case by adding:
https://github.com/mapbox/MapboxStatic.swift/blob/3ab49ce2f8f51baee2895f5bef5b26721c41b5a4/MapboxStatic/Overlay.swift#L399-L425

1ec5

Thanks for taking this one. Apart from the feedback below, the changelog entry should mention both the addition of data task methods and the fact that these methods now resume their tasks, so the postcondition is no longer required.

1ec5 · 2019-02-21T21:40:05Z

MapboxDirections/OfflineDirections.swift

-    @discardableResult
-    func fetchAvailableOfflineVersions(completionHandler: @escaping OfflineVersionsHandler) -> URLSessionDataTask
+    @objc(fetchAvailableOfflineVersionsWithCompletion:)
+    func fetchAvailableOfflineVersions(completionHandler: @escaping OfflineVersionsHandler) -> ()


I think it’s still useful to return a data task, as the “calculate” methods in Directions do. That way the application can cancel the task if necessary. Sort of the “scheduled timer” pattern.

oh, I missed this comment earlier. does that render this PR (and #352) moot @1ec5 ?

No, I think the important thing is that this method resumes the task. It’s fine that it also returns the task.

MapboxDirections/OfflineDirections.swift

akitchen · 2019-02-21T22:03:45Z

I'm not so sure the JSON keys sort order is a factor here - the stub is matching on:

        let queryParams: [String: String?] = [
            "alternatives": "true",
            "geometries": String(describing: shapeFormat),
            "overview": "full",
            "steps": "true",
            "continue_straight": "true",
            "access_token": BogusToken,
        ]
        stub(condition: isHost("api.mapbox.com")
            && isPath("/directions/v5/mapbox/driving/-122.42,37.78;-77.03,38.91.json")
            && containsQueryParams(queryParams)) { _ in
[...]

... so I interpret the expectation timeout to mean that it isn't set up, or that internal async within the URL loading system tearing down the previous stubs caught this one in its dragnet as well.

Since these stubs are so specific and localized, I think we can let them live longer and tear them down at the end of this test class's execution. Small commit coming to test this theory.

akitchen · 2019-02-21T22:18:45Z

Interestingly, the test still failed with eeb41aa (which passes locally). 🤔

akitchen · 2019-02-21T23:39:52Z

I reverted the change to test teardown since it seems to incur the same level of flakiness. We can move the discussion of JSON key sorting into #354 and try to keep this focused and get it merged, or not merged, depending on #353 (comment) (cc @1ec5 )

1ec5 · 2019-03-05T01:50:53Z

MapboxDirections/OfflineDirections.swift

-    @discardableResult
-    func downloadTiles(in coordinateBounds: CoordinateBounds, version: OfflineVersion, session: URLSession?, completionHandler: @escaping OfflineDownloaderCompletionHandler) -> URLSessionDownloadTask
+    @objc(downloadTilesInCoordinateBounds:withVersion:completionHandler:)
+    func downloadTiles(in coordinateBounds: CoordinateBounds, version: OfflineVersion, completionHandler: @escaping OfflineDownloaderCompletionHandler) -> ()


Either the argument label in Swift should be with: or the selector piece in Objective-C should be just version:.

@1ec5 should we plan a major release for this?

[#352]

1ec5 · 2019-06-24T22:10:45Z

Sources/MapboxDirections/OfflineDirections.swift

+     */
+    @discardableResult
+    @objc(availableVersionsDataTaskWithCompletionHandler:)
+    public func availableVersionsDataTask(completionHandler: @escaping OfflineVersionsHandler) -> URLSessionDataTask {
        let task = URLSession.shared.dataTask(with: availableVersionsURL) { (data, response, error) in
            if let error = error {
                return completionHandler(nil, error)


availableVersionsDataTask(_:) still confuses me; I don’t really see the point in keeping it separate. In general, for any kind of API request, the developer would only ever need two kinds of methods:

A method that encapsulates requesting and processing a response, taking a completion handler and returning a (discardable) task for cancellation.

A method or property that provides access to the request payload itself without side effects.

These requirements are satisfied by fetchAvailableOfflineVersions(_:) and availableVersionsURL, and by downloadTiles(in:version:) and tilesURL(for:version:). availableVersionsDataTask(_:) and routingTilesDataTaskWith(_:version:completionHandler:) are confusing hybrids, because they look like accessors or factory methods but take completion handlers, as if they’re the methods that perform the task. In fact, completionHandler is documented as being “called when the request completes”, but calling the method by itself doesn’t cause the completion handler to ever get called.

We can leave these methods in, but we should probably remove @discardableResult because the methods do nothing on their own.

I agree. The hybrid methods don't improve readability or versatility so I folded them into their counterparts.

frederoni · 2019-06-28T15:30:39Z

ready for review
cc @1ec5

akitchen requested review from frederoni and 1ec5 February 21, 2019 05:23

frederoni reviewed Feb 21, 2019

View reviewed changes

MapboxDirectionsTests/OfflineDirectionsTests.swift Outdated Show resolved Hide resolved

frederoni approved these changes Feb 21, 2019

View reviewed changes

akitchen mentioned this pull request Feb 21, 2019

Flaky URLProtocol-based test #354

Closed

1ec5 reviewed Feb 21, 2019

View reviewed changes

akitchen force-pushed the akitchen/adjust-offline-tile-methods branch from cd1f4d1 to fff15d4 Compare February 21, 2019 23:36

akitchen force-pushed the akitchen/adjust-offline-tile-methods branch from fff15d4 to 2bd8a14 Compare February 25, 2019 22:55

1ec5 mentioned this pull request Feb 27, 2019

Add Pause/Resume and progress updates to Directions.downloadTiles #306

Closed

1ec5 reviewed Mar 5, 2019

View reviewed changes

akitchen and others added 5 commits June 24, 2019 20:36

Adjust naming of offline tile downloading methods

1f6cdda

[#352]

Switch test (back) to using trailing closure syntax

67640dd

Add .idea to .gitignore

9115158

Adjusts ObjC method naming in favor of completionHandler and adds docs

d1c32c0

Return and resume data task

3b873cc

frederoni force-pushed the akitchen/adjust-offline-tile-methods branch from ec2f3f6 to 3b873cc Compare June 24, 2019 18:36

Add a blurb in the CHANGELOG

385bbf1

1ec5 reviewed Jun 24, 2019

View reviewed changes

Remove hybrid methods

3ce019f

1ec5 approved these changes Jul 15, 2019

View reviewed changes

1ec5 merged commit ade12fb into master Jul 15, 2019

1ec5 deleted the akitchen/adjust-offline-tile-methods branch July 15, 2019 17:37

1ec5 added the backwards incompatible changes that break backwards compatibility of public API label Jul 27, 2019

1ec5 mentioned this pull request Dec 27, 2019

testGeoJSON() fails intermittently (probably slow decoding) #395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust naming of offline tile downloading methods #353

Adjust naming of offline tile downloading methods #353

akitchen commented Feb 21, 2019

frederoni commented Feb 21, 2019

akitchen commented Feb 21, 2019 •

edited

frederoni commented Feb 21, 2019

1ec5 left a comment

1ec5 Feb 21, 2019 •

edited

akitchen Feb 21, 2019

1ec5 Feb 22, 2019

akitchen commented Feb 21, 2019

akitchen commented Feb 21, 2019

akitchen commented Feb 21, 2019

1ec5 Mar 5, 2019

andrlee Jun 24, 2019

1ec5 Jun 24, 2019

frederoni Jun 25, 2019

frederoni commented Jun 28, 2019

Adjust naming of offline tile downloading methods #353

Adjust naming of offline tile downloading methods #353

Conversation

akitchen commented Feb 21, 2019

frederoni commented Feb 21, 2019

akitchen commented Feb 21, 2019 • edited

frederoni commented Feb 21, 2019

1ec5 left a comment

Choose a reason for hiding this comment

1ec5 Feb 21, 2019 • edited

Choose a reason for hiding this comment

akitchen Feb 21, 2019

Choose a reason for hiding this comment

1ec5 Feb 22, 2019

Choose a reason for hiding this comment

akitchen commented Feb 21, 2019

akitchen commented Feb 21, 2019

akitchen commented Feb 21, 2019

1ec5 Mar 5, 2019

Choose a reason for hiding this comment

andrlee Jun 24, 2019

Choose a reason for hiding this comment

1ec5 Jun 24, 2019

Choose a reason for hiding this comment

frederoni Jun 25, 2019

Choose a reason for hiding this comment

frederoni commented Jun 28, 2019

akitchen commented Feb 21, 2019 •

edited

1ec5 Feb 21, 2019 •

edited