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

always include a walk/vehicleRentalState=null state in initial states #6244

Conversation

tkalvas
Copy link
Contributor

@tkalvas tkalvas commented Nov 8, 2024

Summary

Alternative fix for #6054 .

Generate an initial state with currentMode = WALK, vehicleRentalState = null in addition to the others, when requestMode.includesRenting() and arriveBy = true.

I think this solution changes some general assumptions in what states are possible, and should perhaps be discarded purely on that basis. Earlier, I think it was the case that if requestMode.includesRenting(), vehicleRentalState is never null, it's always either BEFORE_RENTING, HAVE_RENTED, RENTING_FROM_STATION, or RENTING_FLOATING, and the expectation was that it is allowed for BEFORE_RENTING or HAVE_RENTED to reach the goal without ever actually have been renting anything, and it's still a success. Now, it can be null, and it is possible that for correctness every state.getRequest().mode().includesRenting() should be state.getRequest().mode().includesRenting() && state.getVehicleRentalState() != null instead. I only changed the one at the beginning of StreetEdge.traverse, and it seems to work, but it feels incomplete.

Issue

Closes #6054

Unit tests

Because of the above mentioned expectation on the link between includesRenting and vehicleRentalState, some unit test changes were forced.

Documentation

None.

Changelog

Yes, fixes a real bug.

Bumping the serialization version id

No.

@tkalvas tkalvas requested a review from a team as a code owner November 8, 2024 13:00
@tkalvas tkalvas marked this pull request as draft November 8, 2024 13:00
@tkalvas
Copy link
Contributor Author

tkalvas commented Nov 8, 2024

An alternative fix is at #6233 and I prefer it.

Copy link

codecov bot commented Nov 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.72%. Comparing base (5e7b3a6) to head (987d204).
Report is 163 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6244      +/-   ##
=============================================
- Coverage      69.74%   69.72%   -0.02%     
- Complexity     17646    17665      +19     
=============================================
  Files           2006     2007       +1     
  Lines          75529    75650     +121     
  Branches        7730     7743      +13     
=============================================
+ Hits           52678    52748      +70     
- Misses         20135    20187      +52     
+ Partials        2716     2715       -1     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Egress routing with BICYCLE_RENTAL sometimes breaks in GTFS planConnection
2 participants