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

Expose duration_typical via public API. #462

Merged
merged 4 commits into from
Oct 27, 2020

Conversation

MaximAlien
Copy link
Contributor

Closing #459.

@MaximAlien MaximAlien added the improvement Improvement for an existing feature. label Sep 15, 2020
@MaximAlien MaximAlien added this to the v1.1.0 milestone Sep 15, 2020
@MaximAlien MaximAlien self-assigned this Sep 15, 2020
@MaximAlien MaximAlien changed the title Expose duration_typical. Expose duration_typical via public API. Sep 15, 2020
@MaximAlien MaximAlien marked this pull request as draft September 16, 2020 20:23
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good code-wise, with some opportunities for improved documentation.

Suggested changelog entry:

  • Added the DirectionsResult.typicalTravelTime and RouteStep.typicalTravelTime properties that indicate the typical travel time, as opposed to the current expected travel time. (#462)

@@ -139,6 +143,11 @@ open class DirectionsResult: Codable {
*/
open var expectedTravelTime: TimeInterval

/**
The route’s typical travel time, measured in seconds.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention any caveats to this property, such as the ones in the expectedTravelTime documentation above?

  • Does it work in every profile or just driving profiles?
  • Presumably these times are still subject to the vagaries of ferry schedules.
  • The developer shouldn’t assume the user would travel along the route at a fixed speed. Does the Directions API expose an attribute option (attribute) for typical travel times as it does for expected travel times?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added more details about duration_typical:

  • duration_typical is only available when using driving-traffic profile.
  • Tested with route which contains ferry and both duration and duration_typical are affected, mentioned this in docs.
  • I'll mention that user shouldn't make an assumption that travel along the route will be at a fixed speed. I also don't see that duration_typical is supported by AttributeOptions.

@danpaz, not sure if you're the right person to ask, but would it be possible to update documentation in https://docs.mapbox.com/api/navigation/#retrieve-directions to contain duration_typical related info?

Sources/MapboxDirections/RouteStep.swift Show resolved Hide resolved
/**
The route’s typical travel time, measured in seconds.
*/
open var typicalTravelTime: TimeInterval?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is on DirectionsResult rather than Route, so it’s also available on Match. Does the Map Matching API expose this value as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just did a few tests related to Map Matching API and don't see that duration_typical is exposed.

@MaximAlien MaximAlien force-pushed the maxim/459-expose-duration-typical branch from c03738c to aeb6a2e Compare October 6, 2020 17:07
@1ec5 1ec5 changed the base branch from master to main October 6, 2020 17:21
@MaximAlien MaximAlien marked this pull request as ready for review October 6, 2020 17:25
@MaximAlien MaximAlien modified the milestones: v1.1.0, v1.2.0 Oct 26, 2020
@MaximAlien MaximAlien force-pushed the maxim/459-expose-duration-typical branch from e5367be to 380a3e6 Compare October 27, 2020 21:30
@MaximAlien MaximAlien merged commit 6a7ee17 into main Oct 27, 2020
@MaximAlien MaximAlien deleted the maxim/459-expose-duration-typical branch October 27, 2020 21:34
@1ec5 1ec5 modified the milestones: v1.2.0, v1.1.0 Nov 6, 2020
@MaximAlien MaximAlien linked an issue Jul 15, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement for an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publicly expose duration_typical value.
2 participants