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

ISO-8601 date time for GTFS API itinerary responses #5660

Merged

Conversation

leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented Feb 5, 2024

Summary

This PR adds ISO-8601 formatted time responses to the GTFS GraphQL API.

I would like to speak about a few names in the dev meeting before, for example what is the realtime equivalent of the a "scheduled time"? I've called it "actual time" but there are a few other candidates:

  • realtime
  • predicted
  • ???

Unit tests

Added.

Documentation

I would like to have a first round of review before I work more on the documenation.

@leonardehrenfried leonardehrenfried added this to the 2.5 (next release) milestone Feb 5, 2024
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

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

Project coverage is 67.77%. Comparing base (41ffc95) to head (2f763e9).
Report is 35 commits behind head on dev-2.x.

Files Patch % Lines
.../org/opentripplanner/apis/gtfs/GraphQLScalars.java 44.44% 12 Missing and 3 partials ⚠️
...ntripplanner/apis/gtfs/datafetchers/PlaceImpl.java 61.11% 14 Missing ⚠️
...r/ext/fares/impl/CombinedInterlinedTransitLeg.java 0.00% 2 Missing ⚠️
...g/opentripplanner/ext/flex/FlexibleTransitLeg.java 0.00% 2 Missing ⚠️
...pentripplanner/model/plan/FrequencyTransitLeg.java 0.00% 2 Missing ⚠️
...va/org/opentripplanner/model/plan/StopArrival.java 0.00% 2 Missing ⚠️
...ntripplanner/model/plan/UnknownTransitPathLeg.java 0.00% 2 Missing ⚠️
...ipplanner/framework/time/OffsetDateTimeParser.java 94.11% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5660      +/-   ##
=============================================
+ Coverage      67.73%   67.77%   +0.04%     
- Complexity     16424    16505      +81     
=============================================
  Files           1898     1904       +6     
  Lines          72031    72213     +182     
  Branches        7419     7435      +16     
=============================================
+ Hits           48788    48941     +153     
- Misses         20729    20758      +29     
  Partials        2514     2514              

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


type LegTimes {
Copy link
Member Author

Choose a reason for hiding this comment

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

ComplexTime ?
Call ?

Copy link
Member

Choose a reason for hiding this comment

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

Is there some potential feature of a leg time that differs from a "stop time"? Also, does it make sense to always return this type which includes realtime related information even when it's not really feasible that there is any. For example if you walk from origin to a bike rental station? Maybe we should return an union of "realtime times" and "non-realtime times". Are there any other potential use cases for realtime times other than transit? Maybe we should call the realtime version "TransitTime" or "ComplexTime" and the other "SimpleTime"? For some reason, nothing feels perfect. Also, using a union/interface makes the client maybe slightly more complicated.

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 think the best solution would be to make Leg an interface and the implementations define what kind of time it provides.

But we all know that we can only do that with a new schema.

I will be doing some thinking if a union is better or not.

@optionsome
Copy link
Member

Btw I stumpled upon https://github.com/graphql-java/graphql-java-extended-scalars?tab=readme-ov-file#how-to-add-extended-scalars-to-your-schema while researching how to utilize the extended scalars for the new plan query. Maybe we should use the @specifiedBy also.

@leonardehrenfried leonardehrenfried requested a review from t2gran March 1, 2024 13:31
optionsome
optionsome previously approved these changes Mar 1, 2024
@leonardehrenfried
Copy link
Member Author

I will wait to merge this until we released 2.5.0.

@t2gran t2gran modified the milestones: 2.5, 2.6 (next release) Mar 13, 2024
@leonardehrenfried leonardehrenfried merged commit 370191e into opentripplanner:dev-2.x Mar 13, 2024
5 checks passed
@leonardehrenfried leonardehrenfried deleted the iso-datetime branch March 13, 2024 12:57
t2gran pushed a commit that referenced this pull request Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants