Skip to content

Allow limiting the used vehicle rentals and parkings - #3746

Merged
flaktack merged 5 commits into
opentripplanner:dev-2.xfrom
realCity:feature/vehicle-parking-filtering
Nov 23, 2021
Merged

flaktack merged 5 commits into
opentripplanner:dev-2.xfrom
realCity:feature/vehicle-parking-filtering

Conversation

@flaktack

Copy link
Copy Markdown
Contributor

Summary

Four new parameters are added to RoutingRequest and RoutingResource, which allow limiting the used vehicle rental / parking place by network / tags:

  • allowedVehicleRentalNetworks / bannedVehicleRentalNetworks
  • requiredVehicleParkingTags / bannedVehicleParkingTags

A few missing parameters are also added to RoutingResource: bikeParkCost, bikeParkTime, carParkCost, carParkTime.

For vehicle rental:

  • if there are only banned networks, everything not banned is allowed
  • if there are allowed networks, only those may be used
  • if there are both allowed and banned networks, banning takes precedence

For vehicle parking -- tags are currently created from OSM tags (fee, supervised, covered, surveillance):

  • if requiredVehicleParkingTags is specified, than all tags must be present
  • if bannedVehicleParkingTags is specified, than non of the tags may be present

Issue

#3432

Unit tests

☑️

Code style

☑️

Documentation

No changes.

... so that the networks used during planning may be restricted.
... to limit the used vehicle parkings.
… = to be present

Vehicle parking tags may contain an (=) when specified for
requiredVehicleParkingTags / bannedVehicleParkingTags.
@flaktack
flaktack requested a review from a team as a code owner November 15, 2021 13:42
@hannesj
hannesj self-requested a review November 16, 2021 10:54
@t2gran
t2gran requested a review from optionsome November 16, 2021 10:55
@t2gran t2gran added this to the 2.1 milestone Nov 16, 2021

@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.

We at team stadtnavi have been running this code in production for a few months and it's very useful to us, so I approve it being merged.

I picked up on a duplicate method that can potentially be removed.

Comment thread src/main/java/org/opentripplanner/standalone/config/NodeAdapter.java Outdated

@optionsome optionsome 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.

With these current tag options the requiredVehicleParkingTags is probably sufficient. However, I think at some point we might want to allow white listing based on logical or operators too. We could make a separate parameter that expects one of the defined tags to be present but the optimal solution would be to have parameter that would allow to use both "and" and "or" operators. I'm not sure how easy it would be define such format.

@t2gran t2gran added the !New Feature A functional feature targeting the end user. label Nov 18, 2021
@optionsome

Copy link
Copy Markdown
Member

With these current tag options the requiredVehicleParkingTags is probably sufficient. However, I think at some point we might want to allow white listing based on logical or operators too. We could make a separate parameter that expects one of the defined tags to be present but the optimal solution would be to have parameter that would allow to use both "and" and "or" operators. I'm not sure how easy it would be define such format.

We also discussed this today and came to the conclusion that the current implementation is ok for now. If someone has a need for "or" operators we can change the internal model later to support that and implement it in some graphQL API, for example.

@optionsome optionsome 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.

I did purely a code review. I'll probably test at least the vehiclerental parameters soon as I plan to add them to legacy graphql API unless someone else has already done that somewhere/is planning to do it.

@flaktack
flaktack merged commit 44ca9ef into opentripplanner:dev-2.x Nov 23, 2021
t2gran pushed a commit that referenced this pull request Nov 23, 2021
@flaktack
flaktack deleted the feature/vehicle-parking-filtering branch November 22, 2024 09:19
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants