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.

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

Restructure walk/bicycle/car preferences in router-config.json #5582

Merged
merged 18 commits into from
Jan 18, 2024

Conversation

optionsome
Copy link
Member

@optionsome optionsome commented Dec 18, 2023

Summary

Make own wrapper objects for bicycle, car and walk in router-config.json. Additionally, make parking and rental objects for car and triangle, walk, rental and parking objects for bicycle.

Issue

This is related to #4803

Unit tests

Added and updated tests

Documentation

Updated documentation

Changelog

From title

@optionsome optionsome marked this pull request as ready for review January 8, 2024 14:43
@optionsome optionsome requested a review from a team as a code owner January 8, 2024 14:43
@optionsome optionsome added this to the 2.5 (next release) milestone Jan 8, 2024
Copy link

codecov bot commented Jan 8, 2024

Codecov Report

Attention: 27 lines in your changes are missing coverage. Please review.

Comparison is base (ab694b8) 67.55% compared to head (ed51e52) 67.49%.
Report is 60 commits behind head on dev-2.x.

Files Patch % Lines
...ext/restapi/mapping/LegacyBicycleOptimizeType.java 50.00% 6 Missing ⚠️
...rg/opentripplanner/visualizer/GraphVisualizer.java 0.00% 4 Missing ⚠️
.../request/preference/VehicleWalkingPreferences.java 95.16% 0 Missing and 3 partials ⚠️
.../restapi/resources/RequestToPreferencesMapper.java 85.71% 1 Missing and 1 partial ⚠️
...i/request/preference/VehicleRentalPreferences.java 93.10% 0 Missing and 2 partials ⚠️
.../opentripplanner/street/model/edge/StreetEdge.java 75.00% 2 Missing ⚠️
...nner/apis/gtfs/mapping/OptimizationTypeMapper.java 85.71% 1 Missing ⚠️
...routing/api/request/preference/CarPreferences.java 85.71% 0 Missing and 1 partial ⚠️
...ing/api/request/preference/RoutingPreferences.java 0.00% 0 Missing and 1 partial ⚠️
...pi/request/preference/TimeSlopeSafetyTriangle.java 0.00% 0 Missing and 1 partial ⚠️
... and 4 more
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5582      +/-   ##
=============================================
- Coverage      67.55%   67.49%   -0.07%     
- Complexity     16223    16282      +59     
=============================================
  Files           1865     1887      +22     
  Lines          71204    71553     +349     
  Branches        7398     7409      +11     
=============================================
+ Hits           48103    48292     +189     
- Misses         20607    20759     +152     
- Partials        2494     2502       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

/**
* Bicycle optimization types that are only meant to be used by the REST API. Related to {@link org.opentripplanner.routing.core.BicycleOptimizeType}
*/
public enum LegacyBicycleOptimizeType {
Copy link
Member

Choose a reason for hiding this comment

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

👍 Nice separation here.

Comment on lines 380 to 384
// triangle overrides the optimization type if defined
.withForcedOptimizeTriangle(it -> mapOptimizationTriangle(cb, it))
.withWalking(it -> mapVehicleWalking(cb, it))
.withParking(it -> mapParking(cb, it))
.withRental(it -> mapRental(cb, it));
Copy link
Member

Choose a reason for hiding this comment

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

A very nice cleanup. 👍

)
.withRental(it -> setVehicleRental(c, it));
// triangle overrides the optimization type if defined
.withForcedOptimizeTriangle(it -> mapOptimizationTriangle(cb, it))
Copy link
Member

Choose a reason for hiding this comment

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

I'm in favour of this behaviour but doesn't this change how the API works?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only used by the configuration which we can break now. Later on I will use it for the new plan query as well. All the other places use the old method.

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

I have a few comments but generally I think this is a great PR.

…ce/VehicleWalkingPreferences.java

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
@leonardehrenfried
Copy link
Member

leonardehrenfried commented Jan 9, 2024

It's not directly related to this PR but I realised that we don't have a test for mapping the walkRelucance from the API, so I added it: leonardehrenfried@614ef6b

Do you think you can pull it into this branch?

@optionsome
Copy link
Member Author

It's not directly related to this PR but I realised that we don't have a test for mapping the walkRelucance from the API, so I added it: leonardehrenfried@614ef6b

Do you think you can pull it into this branch?

Thanks, I've included it now.

@optionsome optionsome merged commit c8a6b86 into opentripplanner:dev-2.x Jan 18, 2024
5 checks passed
@optionsome optionsome deleted the route-request-refactor branch January 18, 2024 20:52
t2gran pushed a commit that referenced this pull request Jan 18, 2024
t2gran pushed a commit that referenced this pull request Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR Config Change improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants