Skip to content

Conversation

@abhishek1508
Copy link
Contributor

Description

Fixes #5302

Screenshots or Gifs

@abhishek1508 abhishek1508 requested a review from a team as a code owner January 5, 2022 23:57
@abhishek1508 abhishek1508 force-pushed the ak-#5302-use-shields-in-maneuvers branch 2 times, most recently from 30848f4 to b34e4ee Compare January 6, 2022 04:57
Copy link

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

There's a

        userId: String?,
        styleId: String?,
        accessToken: String?,
        maneuvers: List<Maneuver>,
        callback: RoadShieldCallback
    ) {

that we've made public but it's immediately deprecated. Let's make it internal.

@abhishek1508
Copy link
Contributor Author

There's a

        userId: String?,
        styleId: String?,
        accessToken: String?,
        maneuvers: List<Maneuver>,
        callback: RoadShieldCallback
    ) {

that we've made public but it's immediately deprecated. Let's make it internal.

If we make this API internal, how would an end user request mapbox designed shield using deprecated RoadShieldCallback?

@LukasPaczos
Copy link

If we make this API internal, how would an end user request mapbox designed shield using deprecated RoadShieldCallback?

Couldn't they use the new callback instead? They need to make changes to pass in the style id, user, and token, so why not migrate to the new API altogether?

@abhishek1508 abhishek1508 force-pushed the ak-#5302-use-shields-in-maneuvers branch from b34e4ee to 33a030e Compare January 7, 2022 17:03
@abhishek1508
Copy link
Contributor Author

Couldn't they use the new callback instead? They need to make changes to pass in the style id, user, and token, so why not migrate to the new API altogether?

Okay so if I make

fun getRoadShields(
        userId: String?,
        styleId: String?,
        accessToken: String?,
        maneuvers: List<Maneuver>,
        callback: RoadShieldCallback
    )

internal, then that means as a user if you continue to use the deprecated API, you cannot request mapbox designed shields because that API is internal. To be able to use mapbox designed shield migrate to new API. Is that correct?
@LukasPaczos

@LukasPaczos
Copy link

To be able to use mapbox designed shield migrate to new API. Is that correct?

Exactly.

@abhishek1508 abhishek1508 force-pushed the ak-#5302-use-shields-in-maneuvers branch 6 times, most recently from b59b783 to 191a618 Compare January 11, 2022 05:57
binding.maneuverView.renderManeuverWith(shieldResult)
shieldErrors.forEach { (id, errors) ->
errors.forEach { error ->
LoggerProvider.logger.e(

Choose a reason for hiding this comment

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

nit: we can keep logging the error here

@abhishek1508 abhishek1508 force-pushed the ak-#5302-use-shields-in-maneuvers branch 4 times, most recently from aba92b5 to 1003f51 Compare January 11, 2022 23:50
Copy link

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

LGTM with one suggestion.

@abhishek1508 abhishek1508 force-pushed the ak-#5302-use-shields-in-maneuvers branch from 1003f51 to 3bfdd01 Compare January 12, 2022 18:21
@abhishek1508 abhishek1508 force-pushed the ak-#5302-use-shields-in-maneuvers branch from 3bfdd01 to 86cc749 Compare January 12, 2022 19:26
@abhishek1508 abhishek1508 merged commit 2f2961d into main Jan 13, 2022
@abhishek1508 abhishek1508 deleted the ak-#5302-use-shields-in-maneuvers branch January 13, 2022 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use RouteShieldResult and RouteShieldError in maneuvers

2 participants