Skip to content

Add allowOverloading parameter to GBFS updaters and fix use of it in routing - #5024

Merged
optionsome merged 9 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:gbfs-overloading
Apr 13, 2023
Merged

Add allowOverloading parameter to GBFS updaters and fix use of it in routing#5024
optionsome merged 9 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:gbfs-overloading

Conversation

@optionsome

Copy link
Copy Markdown
Member

Summary

This adds optional allowOverloading parameter to the GBFS updaters and fixes use of it in routing.

Issue

This is related to MobilityData/gbfs#376 but unfortunately currently, I don't feel like there is a straightforward way to define this information in the data.

Unit tests

Updated tests.

Documentation

Updated

Changelog

From title

@optionsome optionsome added !Bug Apply to issues describing a bug and PRs witch fixes it. !New Feature A functional feature targeting the end user. labels Apr 3, 2023
@optionsome
optionsome requested a review from a team as a code owner April 3, 2023 14:06
@codecov

codecov Bot commented Apr 3, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 89.47% and project coverage change: +0.02 🎉

Comparison is base (1206bb8) 64.25% compared to head (9835df4) 64.28%.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5024      +/-   ##
=============================================
+ Coverage      64.25%   64.28%   +0.02%     
- Complexity     13786    13791       +5     
=============================================
  Files           1689     1689              
  Lines          66773    66778       +5     
  Branches        7191     7191              
=============================================
+ Hits           42906    42925      +19     
+ Misses         21461    21450      -11     
+ Partials        2406     2403       -3     
Impacted Files Coverage Δ
...kerental/SmooveBikeRentalDataSourceParameters.java 33.33% <ø> (-16.67%) ⬇️
...irectory/VehicleRentalServiceDirectoryFetcher.java 0.00% <ø> (ø)
...kerental/VilkkuBikeRentalDataSourceParameters.java 50.00% <ø> (ø)
...ervice/vehiclerental/model/VehicleRentalPlace.java 66.66% <ø> (ø)
...vice/vehiclerental/model/VehicleRentalVehicle.java 37.50% <ø> (ø)
.../params/GbfsVehicleRentalDataSourceParameters.java 50.00% <ø> (ø)
...g/updaters/sources/VehicleRentalSourceFactory.java 78.00% <60.00%> (+10.65%) ⬆️
...tafetchers/LegacyGraphQLBikeRentalStationImpl.java 70.00% <100.00%> (ø)
...etchers/LegacyGraphQLVehicleRentalStationImpl.java 85.71% <100.00%> (ø)
...t/smoovebikerental/SmooveBikeRentalDataSource.java 100.00% <100.00%> (ø)
... and 4 more

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@optionsome optionsome added the Digitransit Test Feature is under testing in Digitransit environment(s) label Apr 4, 2023
@leonardehrenfried
leonardehrenfried self-requested a review April 4, 2023 08:35
)
implements VehicleRentalDataSourceParameters {
public boolean isAllowOverloading() {
return allowOverloading;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

???

Can you not just call allowOverloading()?

@optionsome

Copy link
Copy Markdown
Member Author

I think I've implemented everything we discussed in the dev meeting. Only thing I'm still unsure about is what to do with the allowOverloading in the GraphQL schema. I don't know if anyone is actually using it so we could maybe just rename it to be overloadingAllowed?

import org.opentripplanner.street.search.state.State;
import org.opentripplanner.transit.model.framework.FeedScopedId;

class VehicleRentalEdgeTest extends GraphRoutingTest {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
class VehicleRentalEdgeTest extends GraphRoutingTest {
class VehicleRentalEdgeTest {

Do you need the GraphRoutingTest?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well spotted. Seems like it's not needed. I used another test class as the basis for mine but it was unnecessary in the other class as well so I removed it from both.

@optionsome optionsome added +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR and removed +Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR labels Apr 6, 2023
@leonardehrenfried

Copy link
Copy Markdown
Member

Please resolve the conflicts.

@optionsome
optionsome merged commit ed6edb1 into opentripplanner:dev-2.x Apr 13, 2023
@optionsome
optionsome deleted the gbfs-overloading branch April 13, 2023 08:26
t2gran pushed a commit that referenced this pull request Apr 13, 2023
@t2gran t2gran added this to the 2.3 milestone Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it. Digitransit Test Feature is under testing in Digitransit environment(s) !New Feature A functional feature targeting the end user.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants