Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encapsulate responses in coherent public types #391

Closed
2 of 8 tasks
1ec5 opened this issue Nov 23, 2019 · 1 comment
Closed
2 of 8 tasks

Encapsulate responses in coherent public types #391

1ec5 opened this issue Nov 23, 2019 · 1 comment
Labels
backwards incompatible changes that break backwards compatibility of public API op-ex Refactoring, Tech Debt or any other operational excellence work.
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Nov 23, 2019

Currently, RouteCompletionHandler and MatchCompletionHandler accept multiple arguments that separate the routes (or matches) from the waypoints they connect. Because these arrays are passed in as top-level arguments to the completion handlers, adding more detail about the response would break backwards compatibility. For this reason, we’ve taken to copying metadata about the response as a whole – namely, the response UUID and options object from the request – into each route or match.

As part of adopting Codable, #382 implements private MapMatchingResponse, MatchResponse, and RouteResponse classes that represent the response as a whole. These classes give us a convenient, central place to put response metadata and neatly parallel MapKit’s MKDirections.Response class. We should make these new classes public and pass them to the completion handler directly instead of dissassembling them for public consumption.

At the same time, we should unify the response classes into two classes: MapMatchingResponse would decode Map Matching API responses (as MapMatchingResponse and MatchResponse both currently do), while RouteResponse would decode Directions API responses. For the case that MapMatchingResponse is meant to handle, it would be better for MapMatchingResponse to produce Match objects; calculateRoutes(matching:completionHandler:) could call a new initializer on RouteResponse that takes a MapMatchingResponse as input.

This refactoring is probably going to be necessary once #388 separates Waypoint into three separate types.

  • Implement decodable response types: Remove Objective-C compatibility; adopt Swift language features #382
  • Make RouteResponse and MatchResponse public: Objective-C removal followups for navigation SDK compatibility #393
  • Take MapMatchingResponse private and internally convert it to RouteResponse
  • Rename MatchResponse to MapMatchingResponse
  • Document RouteResponse and MatchResponse and each of their members
  • Fold an optional error property into RouteResponse and MatchResponse and decode the error as part of decoding these types
  • Make RouteResponse and MatchResponse conform to Encodable?
  • Replace the RouteCompletionHandler and MatchCompletionHandler arguments with RouteResponse and MatchResponse

/cc @mapbox/navigation-ios

@1ec5
Copy link
Contributor Author

1ec5 commented May 29, 2020

Fixed in #406.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards incompatible changes that break backwards compatibility of public API op-ex Refactoring, Tech Debt or any other operational excellence work.
Projects
None yet
Development

No branches or pull requests

2 participants