No Response Fix #314
Merged
No Response Fix #314
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
November 20, 2018 16:51
…ult in edge-case where no error is omitted, but no route objects are returned.
JThramer
commented
Nov 21, 2018
| @@ -1,7 +1,7 @@ | |||
| typealias JSONDictionary = [String: Any] | |||
|
|
|||
| /// Indicates that an error occurred in MapboxDirections. | |||
| public let MBDirectionsErrorDomain = "MBDirectionsErrorDomain" | |||
| public let MBDirectionsErrorDomain = "com.mapbox.directions.ErrorDomain" | |||
There was a problem hiding this comment.
Before anyone asks, this is a great (and rather rare) example of "Do as I say, not as I do" from Apple:
You can create your own error domains and error codes for use in your own frameworks, or even in your own applications. It is recommended that the string constant for the domain be of the form com.company.framework_or_app.ErrorDomain.
|
Thanks for turning this around. Seems like something that should really have a test case, however. Any chance you could add one? |
1ec5
reviewed
Nov 21, 2018
MapboxDirections/MBDirections.swift
Outdated
| @@ -226,7 +226,7 @@ open class Directions: NSObject { | |||
| - returns: The data task for the URL. | |||
| - postcondition: The caller must resume the returned task. | |||
| */ | |||
| fileprivate func dataTask(with url: URL, data: Data? = nil, completionHandler: @escaping (_ json: JSONDictionary) -> Void, errorHandler: @escaping (_ error: NSError) -> Void) -> URLSessionDataTask { | |||
| fileprivate func dataTask(with url: URL, data: Data? = nil, completionHandler: @escaping (_ response: HTTPURLResponse?, _ json: JSONDictionary) -> Void, errorHandler: @escaping (_ error: NSError) -> Void) -> URLSessionDataTask { | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
1ec5
reviewed
Nov 21, 2018
| func testKnownBadResponse() { | ||
| let pass = "The operation couldn’t be completed. Request Entity Too Large" | ||
|
|
||
| OHHTTPStubs.stubRequests(passingTest: { (request) -> Bool in |
There was a problem hiding this comment.
testRateLimitErrorParsing() is a lot more focused, testing just Directions.informativeError(describing:response:underlyingError:) without need to stub out anything.
The recovery message should focus on the freeform parts of the request that are most likely to grow long. Also assert that there’s no underlying error.
1ec5
force-pushed
the
jerrad/no-response-fix
branch
from
3a6919e
to
188e546
Compare
1ec5
approved these changes
Nov 21, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #310, where a situation can arise where an API request returns an error, but that error is not matched by our blacklist and thus the SDK returns the response without an
NSError, but also without a[Route]object. This should never happen./cc @mapbox/navigation-ios