NAVAND-856 NN: adopt set route reason by RingerJK · Pull Request #6620 · mapbox/mapbox-navigation-android

RingerJK · 2022-11-17T12:56:35Z

Description

NavNative: adopt SetRoutesReason

RingerJK · 2022-11-17T13:07:32Z

 import com.mapbox.androidauto.screenmanager.MapboxScreenEvent
 import com.mapbox.androidauto.screenmanager.MapboxScreenManager
 import com.mapbox.maps.logI
-import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI


codecov · 2022-11-17T13:41:16Z

Codecov Report

Merging #6620 (7e33a31) into main (1a7522f) will increase coverage by 0.01%.
The diff coverage is 91.89%.

❗ Current head 7e33a31 differs from pull request most recent head 03f7e4d. Consider uploading reports for the commit 03f7e4d to get more accurate results

@@             Coverage Diff              @@
##               main    #6620      +/-   ##
============================================
+ Coverage     72.31%   72.33%   +0.01%     
+ Complexity     5323     5314       -9     
============================================
  Files           753      754       +1     
  Lines         29089    29074      -15     
  Branches       3448     3447       -1     
============================================
- Hits          21037    21030       -7     
- Misses         6667     6668       +1     
+ Partials       1385     1376       -9

Impacted Files	Coverage Δ
...mapbox/navigation/core/trip/session/TripSession.kt	`100.00% <ø> (ø)`
...gation/navigator/internal/MapboxNativeNavigator.kt	`0.00% <ø> (ø)`
...on/navigator/internal/MapboxNativeNavigatorImpl.kt	`0.00% <0.00%> (ø)`
...avigator/internal/router/RouterInterfaceAdapter.kt	`70.00% <0.00%> (ø)`
.../navigation/core/trip/session/MapboxTripSession.kt	`86.03% <90.62%> (-0.18%)`	⬇️
...ava/com/mapbox/navigation/core/MapboxNavigation.kt	`68.54% <90.90%> (-0.37%)`	⬇️
.../main/java/com/mapbox/navigation/core/SetRoutes.kt	`100.00% <100.00%> (ø)`
...core/directions/session/MapboxDirectionsSession.kt	`92.68% <100.00%> (ø)`
...pbox/navigation/core/internal/utils/SetRoutesEx.kt	`100.00% <100.00%> (ø)`
.../mapbox/navigation/route/internal/RouterWrapper.kt	`81.11% <100.00%> (ø)`
... and 1 more

dzinad · 2022-11-18T12:44:53Z

+    private companion object {
+        private const val DEFAULT_INITIAL_LEG_INDEX = 0
+
+        private fun SetRoutes.initialLegIndex(): Int =


Shouldn't this be in SetRoutesEx?

it's needed only in the particular class, so we're good to keep it here

dzinad · 2022-11-18T12:45:49Z

@@ -0,0 +1,20 @@
+package com.mapbox.navigation.core
+
+internal sealed class SetRoutes {


Is there a specific reason why you replaced SetRoutesInfo with SetRoutes? It looks like it's just doing the same thing differently. But now you have more whens.

They are doing the same, but they are not the same:

SetRoutes is self-defined, it's not needed to hold String Reason

as it's internal, now classes are data and objects

leg index is not present by default anymore: it's not needed for CleanUp and Reroute reasons

This is deep refactoring. I wanted to remove one and define another to show up diff explicitly.

Also, IMO (you can disagree with me 😄 ), I prefer nested sealed classes where it's possible, because they are defined under the same prefix, like

sealed class Base { class Alpha(val value: String) : Base() object Beta : Base() } fun ololo(base: Base) { when(base) { is Base.Alpha -> TODO() Base.Beta -> TODO() } }

vs

sealed class Base class AlphaBase(val value: String) : Base() object BetaBase : Base() fun ololo(base: Base) { when(base) { is AlphaBase -> TODO() BetaBase -> TODO() } }

Yes, but you still have to perform a when to convert it to reason. Why not just use a property?

SetRoutesInfo was also internal;

It's not present anymore by default but now you have to "invent" it for cases where it's not present.

Also, IMO (you can disagree with me 😄 ), I prefer nested sealed classes where it's possible, because they are defined under the same prefix

I don't have a strong preference here.

Yes, but you still have to perform a when to convert it to reason. Why not just use a property?

Whenever we have string property or not we have to perform when to get the carried data.

SetRoutesInfo was also internal;

yes, so we're good to keep them aka data-object

It's not present anymore by default but now you have to "invent" it for cases where it's not present.

it's actually a gap in our API: we don't need LEG INDEX when it's not needed. The most common sample is CleanUp routes - we don't need a leg index here, but setting it, as API is required it. Seems not setting the leg index in the upper level is safe, to not pass it occasionally

dzinad · 2022-11-18T12:47:16Z

+        }
+        logD(LOG_CATEGORY, logMessage("- starting"))
+
+        val result = when (setRoutes) {


Here you have another when to get initialLegIndex, although you've introduced an extension method for this purpose.

when is needed not only for the leg index but also to resolve reason. Also, if you have a look at L96-137. It's not just getting the leg index.

I understand that it's not just about the index. But it's about it as well. And as a result you have 2 places where the logic of getting leg index is implemented.

discussed: move SetRoutes.initialLegIndex() to SetRoutesEx and cut the NAVAND-910 to address DirectionsSession#initialLegIndex concerns

dzinad · 2022-11-18T12:48:33Z

-                "route IDs: ${routes.map { it.id }}) - finished",
-            LOG_CATEGORY
-        )
+        logD(LOG_CATEGORY, logMessage("- finished"))


Why can't you just create a String?

There might be both. Why do you think one is better than another?

Why create a lambda that creates a string when you can just create a string? It will be created anyway so I see adding a lambda as an unnecessary complication.

It's a micro-performance (lazy) upgrade, I would keep it as is. Don't see a real matter to change to the field.

Oh, I didn't notice you were reusing it. OK then.

dzinad · 2022-11-21T13:40:15Z

    private var routesUpdateReason: String = RoutesExtra.ROUTES_UPDATE_REASON_CLEAN_UP

-    override var initialLegIndex = 0
+    override var initialLegIndex = DEFAULT_INITIAL_LEG_INDEX


Did you have a chance to create a ticket to do something with this property?

NAVAND-910

RingerJK added the skip changelog Should not be added into version changelog. label Nov 17, 2022

RingerJK changed the title ~~NN: adopt set route reason~~ NAVAND-856 NN: adopt set route reason Nov 17, 2022

RingerJK commented Nov 17, 2022

View reviewed changes

RingerJK force-pushed the kyv-adopt-NN-api-set-routes-reason branch from 02c56fd to ab8463a Compare November 18, 2022 12:31

RingerJK marked this pull request as ready for review November 18, 2022 12:34

RingerJK requested a review from a team as a code owner November 18, 2022 12:34

RingerJK self-assigned this Nov 18, 2022

dzinad reviewed Nov 18, 2022

View reviewed changes

dzinad reviewed Nov 21, 2022

View reviewed changes

dzinad approved these changes Nov 21, 2022

View reviewed changes

NN: adopt set route reason

03f7e4d

RingerJK force-pushed the kyv-adopt-NN-api-set-routes-reason branch from 7e33a31 to 03f7e4d Compare November 21, 2022 14:18

RingerJK merged commit d4a714d into main Nov 21, 2022

RingerJK deleted the kyv-adopt-NN-api-set-routes-reason branch November 21, 2022 15:30

		@@ -0,0 +1,20 @@
		package com.mapbox.navigation.core

		internal sealed class SetRoutes {

Conversation

RingerJK commented Nov 17, 2022

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Nov 17, 2022 •

edited

Loading