search

mapbox / mapbox-directions-swift

Traffic-aware directions and map matching in Swift on iOS, macOS, tvOS, watchOS, and Linux
https://www.mapbox.com/navigation/
ISC License
183 stars 88 forks source link

Incidents refreshing #704

Closed Udumft closed 2 years ago

Udumft commented 2 years ago

This PR adds Route.refreshLegIncidents method to refresh incidents data. Unit tests updated

Udumft commented 2 years ago

PR has a breaking change:

  💔 API breakage: var RouteLegRefreshSource.refreshedIncidents has been added as a protocol requirement

I don't think there is a way to avoid that, since the meaning of this PR is to add more data to be refreshed.

S2Ler commented 2 years ago

PR has a breaking change:

  💔 API breakage: var RouteLegRefreshSource.refreshedIncidents has been added as a protocol requirement

I don't think there is a way to avoid that, since the meaning of this PR is to add more data to be refreshed.

Can't decide whether it is acceptable breaking change or not, but here it is a doc that describe how to accept the change: https://github.com/mapbox/mapbox-directions-swift/tree/main/swift-package-baseline#the-procedure-of-accepting-breaking-changes

chezzdev commented 2 years ago

I don't think there is a way to avoid that, since the meaning of this PR is to add more data to be refreshed.

Not sure if this tool will accept this, but technically if we would provide a default implementation for the getter, this property will no longer be required to implement.

Udumft commented 2 years ago

But should we add a default value? I'd like to avoid defaulting everything we add. I guess not many users actually implement RouteLegRefreshSource. Do we know know someone who definitely does that? If so, though it is a breaking change, it is quite simple to update for it.

chezzdev commented 2 years ago

This particular protocol may not be implemented by users, but it's much easier to stick to the rule than think about it every time. If we have the chance to avoid breaking the API, I'd try it.

1ec5 commented 2 years ago

Not sure if this tool will accept this, but technically if we would provide a default implementation for the getter, this property will no longer be required to implement.

Yes, standard practice in Cocoa is that protocol members added after the fact are optional. Since optionality doesn’t exist in Swift protocols, the navigation SDK supplies default implementations. We should take that approach here to be consistent.

The navigation SDK also has a mechanism to log to the console when an optional method is unimplemented. There would be nothing wrong with implementing that in this library, but it would be overkill for this protocol that is only theoretically of interest to applications.