-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add openingHours and feeHours to vehicle parkings, and use time restricted routing based on openingHours #3806
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
base: dev-2.x
Are you sure you want to change the base?
Add openingHours and feeHours to vehicle parkings, and use time restricted routing based on openingHours #3806
Conversation
|
Opened this pr as a draft to get comments on what should be changed. We have discussed this topic before in developer meetings and there have already been on suggestions on what should be done (at least at some point in the future). Some things I will change for sure:
Some things to discuss:
|
It is hard to say what would lead to the best performance. A pre-calculated structure may take up more memory space and end up being slower due to fetching data. I think maybe going for the simplest solution now, and then optimize later is a ok strategy. @leonardehrenfried might have some input on how often we encounter this and if it have any performance impact at all. |
| while (iterations < MAX_TIME_RESTRICTION_ITERATIONS) { | ||
| for (final TimeRestrictionWithOffset timeRestriction : timeRestrictions) { | ||
| var offsetFromArrival = timeRestriction.getOffsetInSecondsFromStartOfSearch() - durationInSeconds; | ||
| var timeAtRestriction = time.plusSeconds(offsetFromArrival); | ||
| var traversableAt = timeRestriction.getTimeRestriction() | ||
| .latestArrivalTime(timeAtRestriction); | ||
|
|
||
| if (traversableAt.isEmpty()) { | ||
| break DATETIME_SEARCH; | ||
| } | ||
|
|
||
| var alternateTime = traversableAt.get(); | ||
| if (!alternateTime.equals(timeAtRestriction)) { | ||
| time = alternateTime.minusSeconds(offsetFromArrival); | ||
| iterations++; | ||
| continue DATETIME_SEARCH; | ||
| } | ||
| } |
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.
To me it is not obvious that this loop is safe - do not enter an infinite loop with poor data. If the !alternateTime.equals(timeAtRestriction) is false every time, this loop never exit. Can this be refactored so we by reading the code are sure it is safe?
| <dependency> | ||
| <groupId>io.leonard</groupId> | ||
| <artifactId>opening-hours-evaluator</artifactId> | ||
| <version>1.2.1</version> | ||
| </dependency> |
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 am not sure if we should have this dependency - I know it is @leonardehrenfried and @flaktack who maintain it, but I am skeptical to use a library that is just 2 classes and a thin wrapper around another lib - it is poetically creating problems in the future, while maintaining the needed logic inside OTP seem like a better option. Also the ch.poole.openinghoursparser lib is used in this PR directly, so we should include the dependency to it as well, and not relay on a transitive dependency. Another problem is the license - am not an expert, but we at least need to include it if we use the lib.
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 will keep using this as a depedency but we will add ch.poole.openinghoursparser to pom as an separate depedency as well.
Summary
Add openingHours and feeHours to vehicle parkings and use openingHours for routing.
Issue
relates to #3432
Unit tests
Added tests but they might have to be changed based on the final implementation.
Code style
Have you followed the suggested code style?
yes
Documentation
TODO
Changelog
From title