Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

expose snapping_include_static_closures #1469

Merged
merged 3 commits into from
Aug 8, 2022
Merged

expose snapping_include_static_closures #1469

merged 3 commits into from
Aug 8, 2022

Conversation

dzinad
Copy link
Contributor

@dzinad dzinad commented Jul 28, 2022

No description provided.

@dzinad dzinad requested a review from a team as a code owner July 28, 2022 15:55
@dzinad dzinad force-pushed the dd-1864-snapping-include-static-closures branch from 60cbc03 to 29f3cbe Compare July 28, 2022 15:56
VysotskiVadim
VysotskiVadim reviewed Jul 28, 2022
View reviewed changes
samples/src/main/java/com/mapbox/samples/test.json Outdated
@@ -0,0 +1,122 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

how is this file used? Can't find any usages 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops.

@dzinad dzinad force-pushed the dd-1864-snapping-include-static-closures branch from 29f3cbe to 375020f Compare July 29, 2022 06:19
@codecov
Copy link

codecov bot commented Jul 29, 2022 •
edited
Loading

Codecov Report

Merging #1469 (53f5f95) into main (0e9ea0e) will increase coverage by 0.70%.
The diff coverage is 90.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1469      +/-   ##
============================================
+ Coverage     75.68%   76.38%   +0.70%     
- Complexity      881      896      +15     
============================================
  Files           125      125              
  Lines          3907     3905       -2     
  Branches        578      568      -10     
============================================
+ Hits           2957     2983      +26     
+ Misses          687      675      -12     
+ Partials        263      247      -16
Impacted Files Coverage Δ
.../mapbox/api/directions/v5/models/RouteOptions.java 95.00% <88.88%> (+9.13%) ⬆️
...com/mapbox/api/directions/v5/MapboxDirections.java 89.25% <100.00%> (+0.18%) ⬆️
...com/mapbox/api/directions/v5/utils/ParseUtils.java 93.02% <0.00%> (+4.65%) ⬆️
...om/mapbox/api/directions/v5/utils/FormatUtils.java 83.09% <0.00%> (+12.67%) ⬆️

VysotskiVadim
VysotskiVadim approved these changes Jul 29, 2022
View reviewed changes
Copy link
Contributor

@VysotskiVadim VysotskiVadim left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Please check the comment if you get a chance

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java Outdated
Comment on lines 1934 to 1943
@NonNull
public Builder snappingIncludeStaticClosuresList(
@Nullable List<Boolean> snappingStaticClosures
) {
String result = FormatUtils.join(";", snappingStaticClosures);
if (result != null) {
snappingIncludeStaticClosures(result);
}
return this;
}
Copy link
Contributor

@VysotskiVadim VysotskiVadim Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What does happen if you reset parameters from existing route options?

RouteOptions.builder().
    .build()
     // all all required parameters
    .snappingIncludeStaticClosuresList(listOf(true, false))
    .toBuilder()
    .snappingIncludeStaticClosuresList(null)
    .build()

or

RouteOptions.builder().
     .build()
     // all all required parameters
    .snappingIncludeStaticClosuresList(listOf(true, false))
    .toBuilder()
    .snappingIncludeStaticClosuresList(emptyList())
    .build()

?
Do you think it makes sense to add tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added tests for null and empty list but without previous toBuilder invocation. I also added tests that nothing breaks after toBuilder + build (is this redundant?). I'm not sure tho why it would make sense to check resetting.

Copy link
Contributor

Choose a reason for hiding this comment

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

resetting from a value to null looks suspicious in the code. It seems that in case of null snappingIncludeStaticClosures won't be called from snappingIncludeStaticClosuresList and state won't change. As I see, coordinates work in the same way, so I'm not sure it's a bug. I think it would be nice to document that behaviour in the test. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! I'd say it's a bug. @LukasPaczos added this code, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

That does look like a bug.

RingerJK
RingerJK reviewed Jul 29, 2022
View reviewed changes
Copy link
Contributor

@RingerJK RingerJK left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM with 1 test request

services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/RouteOptions.java Show resolved Hide resolved
@dzinad dzinad force-pushed the dd-1864-snapping-include-static-closures branch from 375020f to a44de32 Compare July 29, 2022 11:44
@LukasPaczos
Copy link
Member

Should this feature receive the same treatment as snapping_include_closures? Should we append it by default for re-routes' origin? I would imagine so but wanted to double check @mandeepsandhu @mskurydin.

@mandeepsandhu
Copy link

Should this feature receive the same treatment as snapping_include_closures? Should we append it by default for re-routes' origin? I would imagine so but wanted to double check @mandeepsandhu @mskurydin.

I believe so, yes. @nuttert can you confirm?

@LukasPaczos LukasPaczos mentioned this pull request Aug 3, 2022
Closed
@LukasPaczos
Copy link
Member

Tracking in mapbox/mapbox-navigation-android#6119.

LukasPaczos
LukasPaczos approved these changes Aug 8, 2022
View reviewed changes
Copy link
Member

@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

@dzinad dzinad merged commit 9ffd5ae into main Aug 8, 2022
@dzinad dzinad deleted the dd-1864-snapping-include-static-closures branch August 8, 2022 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VysotskiVadim VysotskiVadim VysotskiVadim approved these changes

@LukasPaczos LukasPaczos LukasPaczos approved these changes

@RingerJK RingerJK Awaiting requested review from RingerJK

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dzinad @LukasPaczos @mandeepsandhu @VysotskiVadim @RingerJK