-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add previousLegs into GTFS GraphQL API #6142
Add previousLegs into GTFS GraphQL API #6142
Conversation
5d67bf1
to
124f589
Compare
This has some merge conflicts. Can you fix them? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6142 +/- ##
=============================================
- Coverage 69.91% 69.71% -0.21%
+ Complexity 17736 17685 -51
=============================================
Files 2006 2008 +2
Lines 75526 75806 +280
Branches 7730 7759 +29
=============================================
+ Hits 52804 52847 +43
- Misses 20036 20247 +211
- Partials 2686 2712 +26 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could try adding GraphQL integration tests for this field in https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/application/src/test/resources/org/opentripplanner/apis/gtfs/queries/planConnection-extended.graphql . I'm not sure how difficult it is, you might need to edit https://github.com/opentripplanner/OpenTripPlanner/blob/dev-2.x/application/src/test/java/org/opentripplanner/apis/gtfs/GraphQLIntegrationTest.java as well.
The integration test uses a fake, hardcoded routing response from a stub routing service which has no relationship to the stub transit service, which makes me impossible to cross reference between these two in tests. These fetchers are basically untestable in the current setup because it queries the transit service using something returned from the routing service. |
return nextOrPreviousLegs(false); | ||
} | ||
|
||
private DataFetcher<Iterable<Leg>> nextOrPreviousLegs(boolean includeDepartBefore) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This includeDepartBefore
is a slightly difficult to understand boolean name (I know the name is copied over from the getAlternativeLegs
method) but I think you could give it some more understandable name like previousLegsOnly
. In general, we try to avoid boolean method variables that change the behavior. Instead, we prefer to split the methods into two different methods where the name reflects what it does. However, in this case you are just repeating the design decision made in the getAlternativeLegs
method so it might be ok. We can discuss this in a developer meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this in the developer meeting and came to the conclusion that you should split this private method and also getAlternativeLegs into two methods. One for previous legs and one for next legs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previousLegs
and nextLegs
are doing the same thing, with the only difference in direction. I don't want to duplicate codes so I will switch to an enum instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum isn't really any better than boolean. You can put some of the code that computes the other arguments into methods that can be used by both the nextLegs and previousLegs methods so you can avoid some duplication that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I can do anything better than converting the boolean to an enum, as the result will be much more complicated.
The single flag is used for multiple purposes:
- to sort the departures within a pattern
- to filter the results to be before / after the requested time
- to sort the leg results after combining them from different patterns
If I don't use such a flag, I will need two more parameters for generateLegs
, one to sort the results, one to compare the stop times in the priority queue. I will also need another parameter for getAlternativeLegs
to sort the legs across different trip patterns as well, so it will be 3 different interrelated new arguments passed down the call stack instead of a single flag, if these arguments become out of sync strange behavior will result.
nextLegs
and previousLegs
are basically the same thing. They are not different behavior. The same argument can be said of journey planning by departure time or arrival time which should be the same logic, yet I am seeing bugs which happens on arrival only but not departure (#6102).
Similarly, there are other flags in getAlternativeLegs
like exactOriginStop
, exactDestinationStop
, filter
which can't be split into different methods as well.
I'll need to discuss the introduction of an utility enum |
application/src/main/java/org/opentripplanner/routing/alternativelegs/AlternativeLegs.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/routing/alternativelegs/AlternativeLegs.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/routing/alternativelegs/AlternativeLegs.java
Outdated
Show resolved
Hide resolved
93726ae
to
a3f752d
Compare
a3f752d
to
e129934
Compare
ac912a6
to
5ae1e2e
Compare
5ae1e2e
to
64d3978
Compare
application/src/main/java/org/opentripplanner/routing/alternativelegs/AlternativeLegs.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/routing/alternativelegs/AlternativeLegs.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/routing/alternativelegs/AlternativeLegs.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/routing/alternativelegs/AlternativeLegs.java
Outdated
Show resolved
Hide resolved
application/src/main/java/org/opentripplanner/routing/alternativelegs/NavigationDirection.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace the variable name.
PR Instructions
Add previousLegs into GTFS GraphQL API in parallel of nextLegs. This already exists in Transmodel API.
Issue
Closes #6139
Unit tests
The fetcher code is not tested at all.
Documentation
Added to GraphQL schema