-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add validation for visit via locations with coordinates when attempting to link them to graph #6972
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?
Conversation
…cesContainer into LinkingContext
… TemporaryVerticesContainer
…nd TemporaryVerticesContainer
…emporaryVerticesContainer
…e unused temporary edges
…ng.linking package.
We need to figure out what to do with the tests. I feel like there are currently too many of them in too many places and a lot of them are not really that well implemented. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6972 +/- ##
=============================================
+ Coverage 72.11% 72.26% +0.14%
- Complexity 19764 19912 +148
=============================================
Files 2151 2160 +9
Lines 80008 80231 +223
Branches 8072 8093 +21
=============================================
+ Hits 57698 57976 +278
+ Misses 19455 19398 -57
- Partials 2855 2857 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
||
private LinkingContext createLinkingContext(TemporaryVerticesContainer container) { | ||
var linkingRequest = LinkingContextRequestMapper.map(request); | ||
return serverContext.linkingContextFactory().create(container, linkingRequest); |
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 put this factory in the server context. I'm a bit on the edge if it actually made sense or not.
application/src/test/java/org/opentripplanner/routing/core/TemporaryVerticesContainerTest.java
Outdated
Show resolved
Hide resolved
* are no longer needed, this class removes the temporary vertices and edges. It implements | ||
* AutoCloseable and the cleanup is automatically done with a try-with-resources statement. | ||
*/ | ||
public class TemporaryVerticesContainer implements AutoCloseable { |
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.
Technically this includes edges, not vertices and it disposes both. Not sure if we should rename it.
9bc41a7
to
172389b
Compare
Summary
This continues the work from #6704
Temporary vertex creation is moved to routing worker so we don't do it separately for access/egress and direct searches. TemporaryVerticesContainer is now used for coordinate visit via locations as well.
Functional changes:
Issue
Relates to #6501
Unit tests
The testing situation should be refactored.
Documentation
Not needed
Changelog
Skipped