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

Eliminate abstract classes #392

Open
1ec5 opened this issue Dec 7, 2019 · 2 comments
Open

Eliminate abstract classes #392

1ec5 opened this issue Dec 7, 2019 · 2 comments
Labels
backwards incompatible changes that break backwards compatibility of public API jira-sync-complete op-ex Refactoring, Tech Debt or any other operational excellence work.
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Dec 7, 2019

We should eliminate the two abstract classes that this library defines, DirectionsOptions and DirectionsResult.

These classes are only abstract by convention due to language constraints. There’s a risk that developers will mistakenly create instances of DirectionsOptions and attempt to use them in place of the similarly named RouteOptions. Making matters worse, these abstract classes implement static functions required by protocols such as Equatable, robbing concrete subclasses of the ability to provide their own implementations: #382 (review).

RouteOptions and MatchOptions are essentially value classes, which don’t benefit from reference semantics, so they should be converted to structures. Converting Waypoint and Tracepoint into structures (#388) paves the way to do the same with RouteOptions and MatchOptions. It will be possible to reuse a single RouteOptions instance for two Directions API requests without worrying about crosstalk between them after modifying Route.options (or RouteResponse.options after #391).

Route and Match are large, complex classes that may need reference semantics, especially once we implement route refreshing: mapbox/mapbox-navigation-ios#2284. We can convert DirectionsResult into a protocol that both Route and Match conform to. There would be quite a bit of duplication between the two classes. We may be able to mitigate some of the duplication by extending DirectionsResult with default method implementations, but that won’t be possible in all cases, due to interdependencies between properties and methods.

/cc @mapbox/navigation-ios

@1ec5 1ec5 added backwards incompatible changes that break backwards compatibility of public API op-ex Refactoring, Tech Debt or any other operational excellence work. labels Dec 7, 2019
@1ec5 1ec5 added this to the v1.0 milestone Dec 7, 2019
@JThramer JThramer self-assigned this Jan 13, 2020
@JThramer
Copy link
Contributor

JThramer commented Apr 1, 2020

Blocked by mapbox/mapbox-navigation-ios#2350, in progress.

@1ec5 1ec5 modified the milestones: v1.0.0, v0.31.0 (v0.40) Apr 14, 2020
@dersim-davaod
Copy link
Contributor

dersim-davaod commented Apr 23, 2020

I'll try to handle the task as we agreed with @1ec5 and @Udumft it's a good starter issue for a newcomer.

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 jira-sync-complete op-ex Refactoring, Tech Debt or any other operational excellence work.
Projects
None yet
Development

No branches or pull requests

4 participants