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

Make vehicleRentalStation query optionally accept id without feed #5411

Merged

Conversation

optionsome
Copy link
Member

Summary

Adds new feature flag GtfsGraphQlApiRentalStationFuzzyMatching to allow matching station ids without feed to ease migration into new query.

Issue

Minor change and no issue.

Unit tests

Added tests that existin functionality works

Documentation

Updated OTP config doc

Changelog

Maybe from title or we skip

@optionsome optionsome requested a review from a team as a code owner October 10, 2023 13:50
@codecov
Copy link

codecov bot commented Oct 10, 2023

Codecov Report

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

Comparison is base (eaf1ea2) 66.87% compared to head (360fc25) 66.87%.
Report is 1 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5411      +/-   ##
=============================================
- Coverage      66.87%   66.87%   -0.01%     
  Complexity     15496    15496              
=============================================
  Files           1800     1800              
  Lines          69793    69801       +8     
  Branches        7353     7353              
=============================================
+ Hits           46674    46676       +2     
- Misses         20662    20666       +4     
- Partials        2457     2459       +2     
Files Coverage Δ
...ntripplanner/framework/application/OTPFeature.java 88.05% <100.00%> (+0.18%) ⬆️
...pplanner/apis/gtfs/datafetchers/QueryTypeImpl.java 22.40% <37.50%> (+0.08%) ⬆️

... and 1 file with indirect coverage changes

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

@@ -304,6 +308,18 @@ private static FareProduct fareProduct(String name) {
);
}

@Nonnull
private static VehicleRentalStation vehicleRentalStation(String name) {
var station = new VehicleRentalStation();
Copy link
Member

Choose a reason for hiding this comment

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

You can use TestVehicleRentalStationBuilder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll change it

/**
* This matches station's feedScopedId to the given string.
*/
private boolean isMatchRentalStationIds(VehicleRentalStation station, String feedScopedId) {
Copy link
Member

Choose a reason for hiding this comment

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

I find this method name a little hard to parse. Can you improve it?

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 completely agree that the method names are awful but I wasn't quite sure if the method name should start with is. Instead of using the word match we could use equal but even then it's as messy. I'm open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

How about matchesStationId or stationIdMatches or stationIdIsMatch?

vesameskanen
vesameskanen previously approved these changes Oct 17, 2023
@optionsome optionsome merged commit 1331f19 into opentripplanner:dev-2.x Oct 27, 2023
5 checks passed
@optionsome optionsome deleted the vehicle-rental-query-ids branch October 27, 2023 07:55
t2gran pushed a commit that referenced this pull request Oct 27, 2023
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.

None yet

4 participants