Skip to content

Matrix API Integration#626

Merged
Udumft merged 6 commits intomainfrom
jill/matrix-419
Jan 11, 2023
Merged

Matrix API Integration#626
Udumft merged 6 commits intomainfrom
jill/matrix-419

Conversation

@jill-cardamon
Copy link
Copy Markdown
Contributor

@jill-cardamon jill-cardamon commented Nov 8, 2021

Fixes #419.

This PR adds a Matrix object to access the Matrix API. Integration is similar to Directions and Isochrones.
Needs to be rebased on top of main.

@jill-cardamon jill-cardamon added improvement Improvement for an existing feature. platform parity matrices Matrix API labels Nov 8, 2021
@jill-cardamon jill-cardamon self-assigned this Nov 8, 2021
@jill-cardamon jill-cardamon marked this pull request as ready for review November 29, 2021 15:27
@MaximAlien
Copy link
Copy Markdown
Contributor

@jill-cardamon, it'd be good to update this PR as it's pretty outdated.

@jill-cardamon jill-cardamon requested a review from 1ec5 April 4, 2022 17:07
@MaximAlien MaximAlien added this to the v2.5 milestone Apr 4, 2022
@MaximAlien
Copy link
Copy Markdown
Contributor

@1ec5, could you please take a look when you have time?

@ShanMa1991 ShanMa1991 modified the milestones: v2.5, v2.6 May 27, 2022
@MaximAlien MaximAlien removed this from the v2.6 milestone Jul 8, 2022
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 15, 2022

Codecov Report

Merging #626 (9d50ad7) into main (bb9fe98) will decrease coverage by 0.95%.
The diff coverage is 75.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #626      +/-   ##
==========================================
- Coverage   85.30%   84.34%   -0.96%     
==========================================
  Files          52       57       +5     
  Lines        4545     4854     +309     
==========================================
+ Hits         3877     4094     +217     
- Misses        668      760      +92     
Impacted Files Coverage Δ
Sources/MapboxDirections/AttributeOptions.swift 90.27% <ø> (ø)
Sources/MapboxDirections/MatrixOptions.swift 65.57% <65.57%> (ø)
Sources/MapboxDirections/MatrixResponse.swift 71.42% <71.42%> (ø)
Sources/MapboxDirections/MatrixError.swift 72.22% <72.22%> (ø)
Sources/MapboxDirections/Matrix.swift 74.73% <74.73%> (ø)
...rces/MapboxDirections/Extensions/URL+Request.swift 95.83% <95.83%> (ø)
Sources/MapboxDirections/Directions.swift 74.09% <100.00%> (-3.54%) ⬇️
Sources/MapboxDirections/Isochrones.swift 77.64% <100.00%> (ø)

@Udumft Udumft requested a review from a team December 15, 2022 13:51
/**
An array of `Waypoints` objects representing locations that will be in the matrix.
*/
public var waypoints: [Waypoint]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we reuse Waypoint for the Matrix API? Judging from the public API documentation, many of the options in Waypoint don’t apply to the Matrix API, either as input or output.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My thought was that using same Waypoint would be useful when after requesting the Matrix, user will pick these same waypoints to build an actual route.

Comment on lines +37 to +49
/**
The coordinates at a given index in the `waypoints` array that should be used as destinations.

By default, all waypoints in the `waypoints` array are used as destinations.
*/
public var destinations: [Int]?

/**
The coordinates at a given index in the `waypoints` array that should be used as sources.

By default, all waypoints in the `waypoints` array are used as sources.
*/
public var sources: [Int]?
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we make a separate Matrix.Waypoint type, then it could have isSource and isDestination properties to avoid the need for maintaining parallel arrays of indices, which introduce the potential for out-of-bounds errors. Otherwise, a subclass of Waypoint might still be valuable as a place to put these properties.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-worked this approach to have separate arrays of waypoints for sources and destinations.

@Udumft
Copy link
Copy Markdown
Contributor

Udumft commented Dec 21, 2022

Detected breaking changes are not related to current PR:

  💔 API breakage: enumelement VisualInstruction.Component.guidanceView has declared type change from (MapboxDirections.VisualInstruction.Component.Type) -> (MapboxDirections.GuidanceViewImageRepresentation, MapboxDirections.VisualInstruction.Component.TextRepresentation) -> MapboxDirections.VisualInstruction.Component to (MapboxDirections.VisualInstruction.Component.Type) -> (MapboxDirections.GuidanceViewImageRepresentation, MapboxDirections.VisualInstruction.Component.TextRepresentation, MapboxDirections.VisualInstruction.Component.GuidanceViewKind?) -> MapboxDirections.VisualInstruction.Component

@Udumft Udumft requested review from a team and 1ec5 December 21, 2022 08:52
@Udumft Udumft merged commit a6866ee into main Jan 11, 2023
@Udumft Udumft deleted the jill/matrix-419 branch January 11, 2023 09:28
@S2Ler
Copy link
Copy Markdown
Contributor

S2Ler commented Jan 16, 2023

Detected breaking changes are not related to current PR:

  💔 API breakage: enumelement VisualInstruction.Component.guidanceView has declared type change from (MapboxDirections.VisualInstruction.Component.Type) -> (MapboxDirections.GuidanceViewImageRepresentation, MapboxDirections.VisualInstruction.Component.TextRepresentation) -> MapboxDirections.VisualInstruction.Component to (MapboxDirections.VisualInstruction.Component.Type) -> (MapboxDirections.GuidanceViewImageRepresentation, MapboxDirections.VisualInstruction.Component.TextRepresentation, MapboxDirections.VisualInstruction.Component.GuidanceViewKind?) -> MapboxDirections.VisualInstruction.Component

Even if breaking change isn't related, it shouldn't be in output after PR is merged. https://github.com/mapbox/mapbox-directions-swift/tree/main/swift-package-baseline#the-procedure-of-accepting-breaking-changes describes what to do to accept a breaking change.

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. matrices Matrix API platform parity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Matrix API

8 participants