Skip to content

Add support for sandboxed realtime vehicle parking updaters#3796

Merged
optionsome merged 27 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:vehicle-parking-updaters
Jan 7, 2022
Merged

Add support for sandboxed realtime vehicle parking updaters#3796
optionsome merged 27 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:vehicle-parking-updaters

Conversation

@optionsome

@optionsome optionsome commented Dec 30, 2021

Copy link
Copy Markdown
Member

Summary

Add support for realtime vehicle parking updaters and refactor data source code to be shared between vehicle rental and vehicle parking updaters. Adds useVehicleParkingAvailabilityInformation query parameter.

Issue

Relates to #3432

Unit tests

Added unit tests.

Code style

Have you followed the suggested code style?
Yes

Documentation

Updated documentation

Changelog

From title

@optionsome optionsome requested a review from a team as a code owner December 30, 2021 13:03
@optionsome optionsome marked this pull request as draft December 30, 2021 13:05

@optionsome optionsome left a comment

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.

Need feedback on the few comments before I'll finish up this pr. I'll also plan to add updater for HSL park and ride in the scope of this pr.

}
for (VehicleParkingUpdaterParameters configItem : config.getVehicleParkingUpdaterParameters()) {
updaters.add(new VehicleParkingUpdater(configItem));
var source = VehicleParkingDataSourceFactory.create(configItem);

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.

Should this be moved into the updater like it is in VehicleRentalUpdater?

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.

We thought it's better to do this outside of the data source constructor. Changed the vehicle rental to be like that too 124c25b


public static DataSource<VehicleParking> create(VehicleParkingUpdaterParameters source) {
switch (source.getSourceType()) {
case KML: return new KmlBikeParkDataSource(source.getUrl(), source.getFeedId(), source.getNamePrefix(), source.isZip());

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.

Should we create different parameter classes for different updaters that extend the generic parameters similarly as in vehicle rental instead of handpicking parameters from one "VehicleParkingUpdaterParameters" into different data source?

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.

We thought it would be better to have different parameter classes for different updaters. We discussed that these parameters shouldn't extend any generic parameter class. However, while doing the changes in 0901f2f, I ended up implementing it so it extends a generic VehicleRentalUpdaterParameters because all the updaters implement PollingGraphUpdaterParameters and have a data source enum value.

Comment thread src/main/java/org/opentripplanner/updater/DataSource.java Outdated
@optionsome optionsome marked this pull request as ready for review December 31, 2021 13:53
@t2gran t2gran self-requested a review January 3, 2022 09:16
@t2gran t2gran added this to the 2.1 milestone Jan 3, 2022
Comment thread docs/sandbox/VehicleParking.md
Comment thread src/ext/java/org/opentripplanner/ext/vehicleparking/hslpark/HslParkUpdater.java Outdated
Comment thread src/ext/java/org/opentripplanner/ext/vehicleparking/hslpark/HslParkUpdater.java Outdated
if (bicycleSpaces == null && carSpaces == null && wheelchairAccessibleCarSpaces == null) {
return null;
}
return createVehiclePlaces(carSpaces, wheelchairAccessibleCarSpaces, bicycleSpaces);

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.

The createVehiclePlaces method can be deleted in favor of the builder.

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.

The method is also used when creating the capacity and there is not that much room for refactoring while still keeping track if the capacity should be null or an instance of VehicleParkingSpaces.

Comment thread src/ext/java/org/opentripplanner/ext/vehicleparking/hslpark/HslParkUpdater.java Outdated
Co-authored-by: Thomas Gran <t2gran@gmail.com>
@t2gran t2gran requested a review from hannesj January 4, 2022 09:46
@t2gran t2gran added !New Feature A functional feature targeting the end user. +Sandbox This will be implemented as a Sandbox feature labels Jan 4, 2022

@leonardehrenfried leonardehrenfried left a comment

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.

This looks really great work.

I think some of that code comes from our downstream fork which we have been running in prod for quite a while so have high confidence that it works well.

Also thanks for the refactoring!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!New Feature A functional feature targeting the end user. +Sandbox This will be implemented as a Sandbox feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants