leonardehrenfried commented 1 month ago

Summary

We discussed in one of the realtime meetings that the GTFS-RT and Siri code needs to be unified. This PR makes one step into that direction by introducing an abstract base class for handling the timetable snaphots. This is then used by both the Siri and GTFS-RT updaters.

Previously their handling of the snapshot was very similar, but not identical: Siri would always initialise a snapshot in the constructor but GTFS-RT would not. This meant that I had to modify the tests ever so slightly to make them pass, reflecting the fact that GTFS-RT now also initialises a snapshot during instantiation.

Further work

While using a base class has improved encapsulation and the separation of input mapping code from "framework code" (many fields are now private to the base class), the buffer is still very much exposed. In further work I would like to improve this further by totally encapsulating the buffer.

In the end I see design like the following:

split Siri/GTFS-RT updating code from the management of the snapshots
instead of base class, there is a timetable snapshot manager taking care of buffering and snapshoting
there is only one snapshot manager per transit model
the Siri/GTFS-RT updaters have a reference to this snapshot manager
Siri/GTFS-RT updaters can then update the snapshots sequentially

Unit tests

Unit tests were updated. Slight modifications were necessary.

Other work

5852 and #5726 also touch on very similar code and I will wait until they are merged before requesting a code review.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 92.85714% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 68.49%. Comparing base (8f632f1) to head (2d86fc6). Report is 1 commits behind head on dev-2.x.

Files	Patch %	Lines
.../updater/trip/AbstractTimetableSnapshotSource.java	95.34%	1 Missing and 1 partial :warning:
...pplanner/ext/siri/SiriTimetableSnapshotSource.java	85.71%	1 Missing :warning:
...a/org/opentripplanner/model/TimetableSnapshot.java	0.00%	0 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## dev-2.x #5853 +/- ## ============================================= + Coverage 68.47% 68.49% +0.01% - Complexity 16698 16703 +5 ============================================= Files 1914 1915 +1 Lines 72673 72652 -21 Branches 7454 7447 -7 ============================================= - Hits 49763 49760 -3 + Misses 20340 20326 -14 + Partials 2570 2566 -4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.