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 route lookup #6378

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

Conversation

eibakke
Copy link
Contributor

@eibakke eibakke commented Jan 10, 2025

Summary

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

The functionality is backwards compatible, though there may be a discussion here that it should not be and that we should instead improve on some of the choices made earlier. For instance whether the longName check should really be a case insensitive prefix match.

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.

@eibakke eibakke requested a review from a team as a code owner January 10, 2025 13:31
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 72.80702% with 31 lines in your changes missing coverage. Please review.

Project coverage is 69.82%. Comparing base (1030408) to head (d5d2bd5).
Report is 109 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...anner/apis/transmodel/TransmodelGraphQLSchema.java 42.85% 24 Missing ⚠️
...planner/transit/service/DefaultTransitService.java 0.00% 3 Missing ⚠️
...t/java/org/opentripplanner/ext/flex/FlexIndex.java 0.00% 1 Missing ⚠️
...ilter/expr/CaseInsensitiveStringPrefixMatcher.java 85.71% 1 Missing ⚠️
...nsit/model/filter/expr/NullSafeWrapperMatcher.java 85.71% 1 Missing ⚠️
...nsit/model/filter/transit/RouteMatcherFactory.java 95.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6378      +/-   ##
=============================================
+ Coverage      69.73%   69.82%   +0.08%     
- Complexity     18024    18085      +61     
=============================================
  Files           2057     2065       +8     
  Lines          76970    77181     +211     
  Branches        7846     7852       +6     
=============================================
+ Hits           53678    53891     +213     
+ Misses         20545    20536       -9     
- Partials        2747     2754       +7     

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

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.

The main code looks very neat but I have a few comments about the test code.

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.

Overall this looks very good - only one small suggestion to consider.

@eibakke eibakke enabled auto-merge (squash) January 24, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants