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

Import Occupancy Status from GTFS-RT Vehicle Positions #5372

Merged
merged 28 commits into from Oct 16, 2023

Conversation

optionsome
Copy link
Member

@optionsome optionsome commented Sep 22, 2023

Summary

  • Expands internal model for OccupancyStatus to include all values from GTFS RT
  • Imports OccupancyStatus from GTFS RT
  • Exposes OccupancyStatus through trip in the GTFS API
  • Renames vehicle position -> realtime vehicle in the internal model
    • Updater still uses the term vehicle position as it interacts with GTFS RT Vehicle Positions
  • Adds supports for fuzzy trip matching in GTFS RT Vehicle Positions updater

Issue

closes #5364

Unit tests

Added

Documentation

Updated but might need slightly more

Changelog

From title

@optionsome optionsome added RealTimeUpdate The issue/PR is related to RealTime updates config change labels Sep 22, 2023
@optionsome optionsome added this to the 2.5 (next release) milestone Sep 22, 2023
@optionsome optionsome requested a review from a team as a code owner September 22, 2023 10:33
@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

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

Comparison is base (b0d9a18) 66.60% compared to head (7793f69) 66.64%.
Report is 106 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5372      +/-   ##
=============================================
+ Coverage      66.60%   66.64%   +0.04%     
- Complexity     15279    15397     +118     
=============================================
  Files           1790     1794       +4     
  Lines          69364    69682     +318     
  Branches        7306     7345      +39     
=============================================
+ Hits           46197    46437     +240     
- Misses         20690    20757      +67     
- Partials        2477     2488      +11     
Files Coverage Δ
...tripplanner/ext/transmodelapi/model/EnumTypes.java 99.27% <100.00%> (+<0.01%) ⬆️
...rg/opentripplanner/apis/gtfs/GtfsGraphQLIndex.java 90.09% <100.00%> (+0.09%) ⬆️
...ripplanner/apis/gtfs/datafetchers/PatternImpl.java 18.75% <100.00%> (+1.78%) ⬆️
...r/apis/gtfs/datafetchers/StopRelationshipImpl.java 62.50% <ø> (+25.00%) ⬆️
...entripplanner/apis/gtfs/datafetchers/TripImpl.java 21.46% <100.00%> (+2.28%) ⬆️
...er/apis/gtfs/datafetchers/VehiclePositionImpl.java 100.00% <100.00%> (+9.09%) ⬆️
...anner/apis/gtfs/generated/GraphQLDataFetchers.java 0.00% <ø> (ø)
...ntripplanner/apis/gtfs/generated/GraphQLTypes.java 10.44% <100.00%> (+0.81%) ⬆️
...opentripplanner/apis/gtfs/model/TripOccupancy.java 100.00% <100.00%> (ø)
...hicles/internal/DefaultRealtimeVehicleService.java 100.00% <100.00%> (ø)
... and 21 more

... and 47 files with indirect coverage changes

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

@optionsome
Copy link
Member Author

Was it that we use the config change label only when we create breaking changes? I will try to avoid doing that here (just need to figure out how to set default for the features).

@optionsome
Copy link
Member Author

We went through the renaming commit issues with @t2gran today and came to the conclusion that it's not worth it to recreate commits to avoid missing history as the problem was only affecting a few small files.

@leonardehrenfried
Copy link
Member

This branch has conflicts.

@leonardehrenfried
Copy link
Member

@sam-hickey-ibigroup You will be interested to hear that realtime occupancy information is coming to OTP.

@leonardehrenfried leonardehrenfried changed the title Import Occupancy Status from GTFS RT Vehicle Positions Import Occupancy Status from GTFS-RT Vehicle Positions Oct 10, 2023
Copy link
Member

@t2gran t2gran left a comment

Choose a reason for hiding this comment

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

Almost there ...

* a module for the repository as well as the service.
*/
@Module
public interface RealtimeVehicleRepositoryModule {
Copy link
Member

Choose a reason for hiding this comment

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

Naming:

public interface RealtimeVehicleRepository {

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm we already have a RealtimeVehicleRepository, this is a different class.

* a module for the service without the repository, which is injected from the loading phase.
*/
@Module
public interface RealtimeVehicleServiceModule {
Copy link
Member

Choose a reason for hiding this comment

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

Rename:

public interface RealtimeVehicleService {

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above

Comment on lines +241 to +243
List<T> dft = (defaultValues instanceof List<T>)
? (List<T>) defaultValues
: List.copyOf(defaultValues);
Copy link
Member

Choose a reason for hiding this comment

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

I would put this into ListUtils (or create CollectionsUtils):

@Nullable
public static <T> toString(Collection<T> c) {
  if(c == null) {
    return null;
  }
  return ((c instanceof List<T> list) ? list : List.copyOf(defaultValues)).toString();
}

Note! This is also handling null. If we should return "[]" and not null is a matter of taste. Both will work here.

Comment on lines 32 to 47
* If vehicle/carriage is not in use / unavailable, or passengers are only allowed to alight due to e.g. crowding
* The vehicle or carriage is not accepting passengers.
Copy link
Member

Choose a reason for hiding this comment

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

Tip: If you do not want to maintain all this doc in multiple places you could use the DocumentedEnum interface.

optionsome and others added 3 commits October 13, 2023 22:12
Co-authored-by: Thomas Gran <t2gran@gmail.com>
…json/ParameterBuilder.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
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 think we want to also generate the documentation for the default values of enum sets but that can come in a separate PR.

@optionsome optionsome merged commit 80647f7 into opentripplanner:dev-2.x Oct 16, 2023
5 checks passed
@optionsome optionsome deleted the gtfsrt-occupancy branch October 16, 2023 12:21
t2gran pushed a commit that referenced this pull request Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RealTimeUpdate The issue/PR is related to RealTime updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support GTFS RT Occupancy Status
3 participants