Skip to content
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

Add a matcher API for filters in the transit service used for regularStop lookup #6234

Open
wants to merge 14 commits into
base: dev-2.x
Choose a base branch
from

Conversation

eibakke
Copy link
Contributor

@eibakke eibakke commented Nov 6, 2024

Summary

This change allows for a clean separation of concerns and lays the path for more efficient filtering logic down the line.

Also moves bounding box filtering on stops from the spatial index into a shared method so that all callers don't have to do this filtering themselves.

Issue

#5630

Unit tests

Added unit tests for each matcher and the new RegularStopMatcherFactory. Also ensured that the API in local runs behaves as expected.

Documentation

Added JavaDoc.

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 30.43478% with 48 lines in your changes missing coverage. Please review.

Project coverage is 69.83%. Comparing base (c7fe2f2) to head (7f5f5e3).
Report is 190 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...anner/apis/transmodel/TransmodelGraphQLSchema.java 31.57% 13 Missing ⚠️
.../request/FindRegularStopsByBoundingBoxRequest.java 0.00% 9 Missing ⚠️
...t/FindRegularStopsByBoundingBoxRequestBuilder.java 0.00% 9 Missing ⚠️
...odel/filter/transit/RegularStopMatcherFactory.java 22.22% 7 Missing ⚠️
...pentripplanner/ext/restapi/resources/IndexAPI.java 0.00% 2 Missing ⚠️
...r/transit/model/filter/expr/ExpressionBuilder.java 0.00% 2 Missing ⚠️
...planner/transit/service/DefaultTransitService.java 50.00% 2 Missing ⚠️
...lanner/ext/parkAndRideApi/ParkAndRideResource.java 0.00% 1 Missing ⚠️
...xt/vectortiles/layers/stops/StopsLayerBuilder.java 0.00% 1 Missing ⚠️
...nner/apis/transmodel/model/stop/StopPlaceType.java 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6234      +/-   ##
=============================================
+ Coverage      69.79%   69.83%   +0.03%     
- Complexity     17790    17910     +120     
=============================================
  Files           2017     2038      +21     
  Lines          76042    76484     +442     
  Branches        7781     7819      +38     
=============================================
+ Hits           53077    53410     +333     
- Misses         20263    20340      +77     
- Partials        2702     2734      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…arStops.

Also moves the envelope filtering that used to happen on the client side of the findRegularStops call into the StopModelIndex.
@eibakke eibakke marked this pull request as ready for review December 3, 2024 14:00
@eibakke eibakke requested a review from a team as a code owner December 3, 2024 14:00
@t2gran t2gran self-requested a review December 4, 2024 00:34
@t2gran t2gran added this to the 2.7 (next release) milestone Dec 4, 2024
…lter/transit/RegularStopMatcherFactoryTest.java

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

Sorry for the back and forth about agency/feed id. This looks ok now.

Copy link
Member

@t2gran t2gran left a comment

Choose a reason for hiding this comment

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

This looks good to me. Very good job on the JavaDoc, but we skip @param and @return if there is no extra information to add. We use them if there are special invariants or things like unit, to document.

…est/FindRegularStopsByBoundingBoxRequest.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…est/FindRegularStopsByBoundingBoxRequestBuilder.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
eibakke and others added 5 commits December 19, 2024 15:16
…lter/transit/RegularStopMatcherFactory.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…lter/transit/RegularStopMatcherFactory.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…TransitService.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…TransitService.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
@eibakke eibakke requested a review from t2gran December 19, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants