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

GTFS Flex spec update: separate columns for location_id, location_group_id #5564

Merged
merged 13 commits into from Dec 19, 2023

Conversation

leonardehrenfried
Copy link
Member

Summary

At the latest GTFS Flex working group we decided that we want to put location_id and location_group_id into their separate columns in order to make it clearer what is a flex trip and what isn't.

This PR catches up with this spec change and uses some Java 21 features to clean up the code a little.

Lastly, since the stop_id column can now be empty, I added

Unit tests

✔️

cc @westontrillium @tzujenchanmbd @tsherlockcraig @eliasmbd @gcamp

@leonardehrenfried leonardehrenfried requested a review from a team as a code owner December 12, 2023 10:23
@leonardehrenfried leonardehrenfried added IBI Developed by or important for IBI Group improvement labels Dec 12, 2023
@leonardehrenfried leonardehrenfried changed the title Flex spec update: separate columns for location_id, location_group_id GTFS Flex spec update: separate columns for location_id, location_group_id Dec 12, 2023
Copy link

codecov bot commented Dec 12, 2023

Codecov Report

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

Comparison is base (4230fd3) 67.35% compared to head (40fcc74) 67.38%.
Report is 1 commits behind head on dev-2.x.

❗ Current head 40fcc74 differs from pull request most recent head cbad217. Consider uploading reports for the commit cbad217 to get more accurate results

Files Patch % Lines
...ntripplanner/gtfs/mapping/LocationGroupMapper.java 28.57% 4 Missing and 1 partial ⚠️
...g/opentripplanner/gtfs/mapping/StopTimeMapper.java 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5564      +/-   ##
=============================================
+ Coverage      67.35%   67.38%   +0.02%     
- Complexity     16162    16169       +7     
=============================================
  Files           1858     1858              
  Lines          71093    71095       +2     
  Branches        7403     7399       -4     
=============================================
+ Hits           47888    47904      +16     
+ Misses         20745    20732      -13     
+ Partials        2460     2459       -1     

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

Copy link
Contributor

@vpaturet vpaturet left a comment

Choose a reason for hiding this comment

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

I need a bit of context regarding the changes in onebusaway-gtfs/src/main/java/org/onebusaway/gtfs/model/StopTime.java, notably the difference between the getters getStop(), getLocation(), .. that go through StopTimeProxy and getStopLocation() that does not go through that proxy.

"Nested GroupStops are not allowed"
);
case null, default -> throw new RuntimeException(
"Unknown location type: " + location.getClass().getSimpleName()
Copy link
Contributor

Choose a reason for hiding this comment

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

location.getClass() triggers a NullPointerException if location == null
Is "case null" actually a possible code path?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've cleaned this up here:
8c3411c

I made the null check more explicit and added a nicer error message.

@leonardehrenfried
Copy link
Member Author

I need a bit of context regarding the changes in onebusaway-gtfs/src/main/java/org/onebusaway/gtfs/model/StopTime.java, notably the difference between the getters getStop(), getLocation(), .. that go through StopTimeProxy and getStopLocation() that does not go through that proxy.

I'm not really sure what the proxy is used for but it only exists for getters for which there is a proper column in GTFS. The getStopLocation on the other hand is just a convenience method that goes through the stop, location or location group columns to find the first non-empty one (previously you used stop_id also for flex stops). Since it doesn't have a corresponding column, it doesn't have a proxy.

@leonardehrenfried leonardehrenfried merged commit e60026e into opentripplanner:dev-2.x Dec 19, 2023
5 checks passed
t2gran pushed a commit that referenced this pull request Dec 19, 2023
@leonardehrenfried leonardehrenfried deleted the flex-spec-update branch December 19, 2023 11:58
@t2gran t2gran added this to the 2.5 (next release) milestone Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IBI Developed by or important for IBI Group improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants