Routable points support for services-geocoding#1522
Conversation
DzmitryFomchyn
force-pushed
the
df-routable-points
branch
2 times, most recently
from
40164e6 to
c94cffc
Compare
Codecov Report
@@ Coverage Diff @@
## main #1522 +/- ##
============================================
+ Coverage 77.25% 77.26% +0.01%
- Complexity 952 957 +5
============================================
Files 130 132 +2
Lines 4080 4091 +11
Branches 587 588 +1
============================================
+ Hits 3152 3161 +9
- Misses 678 679 +1
- Partials 250 251 +1
|
DzmitryFomchyn
force-pushed
the
df-routable-points
branch
from
c94cffc to
33d3c6c
Compare
Guardiola31337
left a comment
Guardiola31337
left a comment
There was a problem hiding this comment.
Left some comments.
| * @return routable point name | ||
| * @since 6.10.0 | ||
| */ | ||
| @Nullable |
There was a problem hiding this comment.
Is this really nullable? I mean, what I'd expect is RoutablePoints to be nullable but If they're not, any RoutablePoint from the list to have valid values. Does that make sense?
There was a problem hiding this comment.
@Guardiola31337 Do you mean by duplicating e.g. the rooftop location in RoutablePoints? If yes, I disagree.
I assume the correct distinction would be to leave RoutablePoints null if there are none and let the app logic decide how to behave in that case (e.g. fallback to original location). Otherwise it's becoming ambiguous whether the data is actually available for the specific search result.
There was a problem hiding this comment.
@Guardiola31337 in addition to what @kalbaxa said, I think it's better to leave it as nullable because all the service-geocoder relies on AutoValue/GSON serialisation/deserialisation. If we declare any field as non-nullable and for some reason backend don't send the value, the SDK will crash. I don't want this. I think it's better to check for null on customer's side and use alternative fallback. Also, nullable fields are consistent with the current approach.
There was a problem hiding this comment.
@kalbaxa What I meant was that routable points could be null (that's totally fine) but if the server emits a routable point, I would expect not to be null (having null fields), that's the contract I'd expect from the backend.
There was a problem hiding this comment.
@DzmitryFomchyn IMO errors in the backend should fail-fast, this approach would hide this type of issues as they'd go unnoticed.
There was a problem hiding this comment.
A can agree with that, but we should also be consistent here. All the current code relies on nullability, it won't make any better if we make one field as non-null while everything else remain nullable. Given everything that's written above, I think we should leave as is now and discuss another approach in case of SDK refactoring, if it happen. More likely, service-geocoder will be deprecated soon.
There was a problem hiding this comment.
If we declare any field as non-nullable and for some reason backend don't send the value, the SDK will crash. I don't want this.
+++
Event if it's not an error on the backend side, it can be a logic change. For example, we accidentally made waypoints nullable but that played well because after some time they did become nullable because a case was introduced that they can be returned in a different response level.
There was a problem hiding this comment.
it can be a logic change
A logic change like this should mean a major version upgrade of an endpoint. All customers (not only Nav SDK) would be impacted by such an intentional change.
There was a problem hiding this comment.
Not necessarily. With waypoints they wouldn't (and weren't). Because it depends on a new request parameter which falls back to old behaviour.
There was a problem hiding this comment.
We would have to provide backwards compatibility if that wasn't the case (either on backend or mapbox-java level). We were lucky and needed less work for that specific situation but that shouldn't be the reason to make contracts less strict going forward.
kalbaxa
left a comment
kalbaxa
left a comment
There was a problem hiding this comment.
When we align with @DzmitryFomchyn and @Guardiola31337 on the routable points approach, I'm good with the merge.
DzmitryFomchyn
force-pushed
the
df-routable-points
branch
from
1ae380d to
a9cdf39
Compare
|
Discussed internally, let's ship as is as |
5 participants
Add routable points support for
services-geocoding