Move PathTransfer to new transfer module #7115
Conversation
jessicaKoehnke
added
!Technical Debt
|
This is close, but only the root access points ( |
jessicaKoehnke
force-pushed
the
transfer-module-refactoring
branch
from
0abc62a to
e1216dc
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #7115 +/- ##
=============================================
+ Coverage 72.05% 72.08% +0.02%
- Complexity 20679 20709 +30
=============================================
Files 2242 2250 +8
Lines 83801 83964 +163
Branches 8348 8347 -1
=============================================
+ Hits 60385 60526 +141
- Misses 20488 20510 +22
Partials 2928 2928 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jessicaKoehnke
force-pushed
the
transfer-module-refactoring
branch
from
f4e5ee6 to
4dc2ef3
Compare
application/src/main/java/org/opentripplanner/standalone/configure/ConstructApplication.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/transit/service/TimetableRepository.java
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/transfer/internal/DefaultTransferRepository.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/transfer/internal/DefaultTransferRepository.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/transfer/internal/DefaultTransferRepository.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/transfer/TransferRepository.java
Show resolved
Hide resolved
| if (OTPFeature.FlexRouting.isOn()) { | ||
| // Flex transfers should only use WALK mode transfers. | ||
| for (PathTransfer transfer : transferRepository.findTransfers(StreetMode.WALK)) { | ||
| transfersToStop.put(transfer.to, transfer); | ||
| transfersFromStop.put(transfer.from, transfer); | ||
| } | ||
| } | ||
| LOG.info("Transfer repository index init complete."); |
There was a problem hiding this comment.
If the indices in this class are specific to flex I think that should be indicated in the names of the collections and the accessor methods. Now it looks like these are indices of all transfers to and from a specific stop.
There was a problem hiding this comment.
I was a bit hesitant on how to refactor this part. I wanted to make sure that the index is only created when flex is on, so that the memory is conserved when it's off. But you do make a good point. I tried doing it with another abstraction layer now. I also moved the flex index code back to the sandbox package, I do get some Class xxx is not exported from module 'org.opentripplanner.otp' warnings from IntelliJ now though. I'm not a hundred percent sure how the dependencies between the sandbox and the core features should be managed best. Is what I did now in line with what we want to aim for in the future?
There was a problem hiding this comment.
Using inheritance instead of an interface might be better for future proving this, I can change it if you think that is a better idea
There was a problem hiding this comment.
I'm not sure what the cleanest way would be to handle the flex specific indices and I don't really have a strong opinion. We do something similar for the timetable repository with FlexIndex, maybe you could mimic that? Perhaps @t2gran has an opinion?
There was a problem hiding this comment.
I have now changed it to using inheritance. This way any future additions to the normal TransferIndex will be automatically adopted by the FlexTransferIndex. Additionally I put the flex feature toggle condition into the application construction code. The business logic is now free of any sandbox dependencies. I think this is a good goal to aim for, that way we can create an application construction module, a sandbox module and several business logic modules in the future and we'd have no circular dependencies.
…refactoring # Conflicts: # application/src/ext-test/java/org/opentripplanner/ext/flex/trip/ScheduledDeviatedTripIntegrationTest.java # application/src/ext/java/org/opentripplanner/ext/flex/FlexIndex.java # application/src/test/java/org/opentripplanner/transit/model/_data/TransitTestEnvironmentBuilder.java
… index from normal index
…dex support any future additions to TransferIndex automatically
…not business logic
3 participants
Summary
This is a refactoring to enable the runtime modification of transfers. PathTransfers are pulled out of the TransitRepository and moved into their own repository. Constrained transfers will be moved into the same module in a next step.
Issue
It's the first step for the plan detailed in issue #7074
Unit tests
No new features and no new tests
Documentation
Bumping the serialization version id
yes