Add Detail API lookup by Mapbox ID #298
Conversation
…569-add-Detail-API-lookup-by-mapbox_id
… Mapbox ID - Refine documentation - SearchEngine.search and SearchEngine.select are a two-step combination - Stop wrapping SearchEngine.retrieve(mapboxID:) in an over-loaded SearchEngine.select wrapper - Mark SearchEngine.retrieve(mapboxID:) public as the main and only way to query Details API / Mapbox ID
…r retrieve(mapboxID:)
…569-add-Detail-API-lookup-by-mapbox_id
…569-add-Detail-API-lookup-by-mapbox_id
… TODO with ticket number
…569-add-Detail-API-lookup-by-mapbox_id Conflicts: MapboxSearch.xcodeproj/project.pbxproj Sources/MapboxSearch/InternalAPI/CoreSearchEngineProtocol.swift Sources/MapboxSearch/PublicAPI/ServiceProvider.swift Tests/MapboxSearchTests/Legacy/SearchEngineTests.swift
…ks for these pathways instead
…Query - Uses mocked data
| func retrieveDetails( | ||
| for mapboxId: String, | ||
| options: CoreDetailsOptions, | ||
| completion: @escaping CoreSearchResponseCompletion | ||
| ) { |
There was a problem hiding this comment.
Is it needed here to return the request id to match the rest of the implementations?
| func retrieveDetails( | |
| for mapboxId: String, | |
| options: CoreDetailsOptions, | |
| completion: @escaping CoreSearchResponseCompletion | |
| ) { | |
| @discardableResult | |
| func retrieveDetails( | |
| for mapboxId: String, | |
| options: CoreDetailsOptions, | |
| completion: @escaping CoreSearchResponseCompletion | |
| ) -> UInt64 { |
|
|
||
| guard let result = responseResult.results.first else { | ||
| let errorMessage = "Could not retrieve details result." | ||
| assertionFailure(errorMessage) |
There was a problem hiding this comment.
I don't think it's a good idea to add assertionFailure here. It does not look like a good practice when a possible backend error triggers the debug crash.
It's better to report error here
| /// - Parameters: | ||
| /// - mapboxID: The Mapbox ID for a known POI. Mapbox IDs will be returned in Search responses and may be cached. | ||
| /// - detailsOptions: Options to configure this query. May be nil. | ||
| public func retrieve(mapboxID: String, options detailsOptions: DetailsOptions? = DetailsOptions()) { |
There was a problem hiding this comment.
Could the parameter detailsOptions be non-optional? Currently, there are 2 places, where the default value is provided. But it could be changed to only 1 place in the method definition if the parameter is non-optional.
| switch response.process() { | ||
| case .success(let responseResult): | ||
| // Response Info is not supported for retrieving by Details API at this time. | ||
| responseInfo = nil |
There was a problem hiding this comment.
The responseInfo is used to report user Feedback to the Telemetry. Could you please elaborate on why we need to reset the responseInfo after retrieve(mapboxID:options:) execution? The app user still can send feedback
| /// Besides the basic metadata attributes, developers can request additional | ||
| /// attributes by setting attribute_sets parameter with attribute set values, | ||
| /// for example &attribute_sets=basic,photos,visit. | ||
| /// The requested metadata will be provided in metadata object in the response. |
There was a problem hiding this comment.
Could you please add the DocC reference to the mentioned metadata property to simplify the understanding where the result can be found
|
|
||
| /// The two digit ISO country code (such as 'JP') to request a worldview for the location data, if applicable data | ||
| /// is available. | ||
| /// This parameters will only be applicable for Boundaries and Places feature types. |
There was a problem hiding this comment.
Could you please elaborate on what are Boundaries and Places types? Are these any Search SDK types?
| @@ -91,7 +91,5 @@ class ServerSearchResult: SearchResult, SearchResultSuggestion, CoreResponseProv | |||
| self.brandID = coreResult.brandID | |||
|
|
|||
| assert(!id.isEmpty) | |||
| assert(!name.isEmpty) | |||
There was a problem hiding this comment.
Is it a valid search response with an empty name?
| XCTAssertNotNil(result.routablePoints?.first) | ||
|
|
||
| XCTAssertNil(metadata.children) | ||
| XCTAssertNotNil(metadata.primaryImage) |
There was a problem hiding this comment.
Do I understand correctly that the response is provided from the local file Tests/MockData/search-box/search-box-retrieve-mapbox-id.json? Why not test the exact values then? It is an integrating test, it would be more useful if it would help to catch the problems in filling specific metadata fields.
…) options parameter to non-optional with a default value
…ssert, fixed with 20b23a
…values in mocked tests
… with Android and Mapbox account configuration
Description
Fixes SSDK-569
SearchEngine.select(mapboxID: String, options: DetailsOptions)to invoke aretrieve/API endpoint call to retreive details from a Mapbox ID.Checklist
CHANGELOG