NAVAND-979: support yards and ios/android thresholds parity #6786
Conversation
dzinad
force-pushed
the
NAVAND-979-dd-yards
branch
from
d7f4ce4 to
fdb327e
Compare
dzinad
force-pushed
the
NAVAND-979-dd-yards
branch
from
331b93d to
e361c64
Compare
| TurfConstants.UNIT_KILOMETERS | ||
| ) | ||
| when { | ||
| distanceInMeters < 3000.0 -> largeValue( |
There was a problem hiding this comment.
the exact values here are a bit confusing. Why these values were chosen (like 1000.0 3000.0)? It's not clear from the code
There was a problem hiding this comment.
as soon as the PR will be merged, it's lost the context of these values. Could you move values to const and add comments 🙏
There was a problem hiding this comment.
It's just that it's not really clear how to call those constants. While here it's more or less already explained in the code.
For example, for this 3000 I can think of a name: MIN_DISTANCE_TO_USE_LARGE_UNITS_AND_1_FRACTION_DIGIT. Would it really help?
There was a problem hiding this comment.
I think so 👍 IMO the same important make leave the comments that the values are in sync with iOS
There was a problem hiding this comment.
I don't really agree here. What if we add a new dimension? For example, determine font size along with units and fraction digits (artificial example, but I can't think of a better one from the top of my head). Should I change the constant name then to MIN_DISTANCE_TO_USE_LARGE_UNITS_AND_1_FRACTION_DIGIT_AND_FONT_SIZE_14?
I think it's redundant. It's all specified in the code in place of usage. We pass it all as arguments.
| if (locale.language == enLanguage && locale.country == "GB") { | ||
| getUKDistance(distanceInMiles, roundingIncrement, locale) | ||
| } else { | ||
| getUSDistance(distanceInMiles, roundingIncrement, locale) |
There was a problem hiding this comment.
I'm not really an expert, but should we take into account other locations (like Australia and maybe some else)? Maybe set a more generic name for this? like getYardDistance.
Also, do we really need to check the language for the GB locale?
There was a problem hiding this comment.
That's a good question. In the ticket only UK was being discussed.
@abhishek1508 @1ec5 are there any other countries that we should use yards in? As far as I understood, on iOS it's just GB, right?
There was a problem hiding this comment.
Checked that on iOS the logic is exactly the same: https://github.com/mapbox/mapbox-navigation-ios/blob/96d0fc0744d64d2a092f571e1583eeb86831a774/Sources/MapboxCoreNavigation/DistanceFormatter.swift#L140
| @@ -105,132 +268,44 @@ object MapboxDistanceUtil { | |||
| * @param unitType indicates whether the value should be returned as metric or imperial | |||
| * @return an object containing values for displaying the formatted distance | |||
| */ | |||
| @JvmOverloads | |||
There was a problem hiding this comment.
wdyt if change the signature to wrapper (builder included) to support future possible changes?
There was a problem hiding this comment.
Do you mean introduce a new method like
formatDistance(Wrapper)
where Wrapper is a model class with builder?
I'm not sure. I mean if we add a new parameter we will probably need it in our calculations. Which means that we will have to provide the default value anyway to be backward compatible. How is Builder going to help here?
There was a problem hiding this comment.
java developers cannot fully rely on default parameters. For instance in the method
@JvmOverloads
fun method(
a: A,
b: B = defaultB,
c: C = defaultC
)java developers cannot overload the default value for param b, and keep the same for c, so invoking of method on the java side would look like (we're forcing to set param c, even if default value exists)
#method(a, customValuesB, defaultC)There was a problem hiding this comment.
I think you meant the other way around.
But OK, agreed that default parameters may be tricky to support in the future so I'll add a new method just the way it's done for the formatDistance that returns FormattedDistanceData.
Codecov Report
@@ Coverage Diff @@
## main #6786 +/- ##
============================================
+ Coverage 72.53% 72.57% +0.04%
+ Complexity 5559 5556 -3
============================================
Files 779 779
Lines 30092 30131 +39
Branches 3547 3551 +4
============================================
+ Hits 21826 21869 +43
+ Misses 6841 6840 -1
+ Partials 1425 1422 -3
|
dzinad
force-pushed
the
NAVAND-979-dd-yards
branch
from
450307a to
1d8116f
Compare
This PR:
Rounding increment parity is not supported here, it will be addressed separately.