Skip to content

Move nearby stop and place finder classes to a new package and move nearby stop finding to happen in one interface#7565

Merged
optionsome merged 2 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:move-nearby-stop-finder
May 12, 2026
Merged

Move nearby stop and place finder classes to a new package and move nearby stop finding to happen in one interface#7565
optionsome merged 2 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:move-nearby-stop-finder

Conversation

@optionsome
Copy link
Copy Markdown
Member

@optionsome optionsome commented Apr 27, 2026

Summary

Moves nearbystopfinder classes away from graph builder.

Issue

No issue

Unit tests

No functional changes

Documentation

Not updated

Changelog

Skipped

@optionsome optionsome added this to the 2.10 (next release) milestone Apr 27, 2026
@optionsome optionsome requested a review from a team as a code owner April 27, 2026 20:33
@optionsome optionsome added !Technical Debt Improve code quality, no functional changes. +Skip Changelog This is not a relevant change for a product owner since last release. labels Apr 27, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 68.00000% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.55%. Comparing base (9f72a3a) to head (2b031b9).
⚠️ Report is 1 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...r/ext/transferanalyzer/DirectTransferAnalyzer.java 0.00% 6 Missing ⚠️
...entripplanner/apis/transmodel/support/GqlUtil.java 0.00% 6 Missing ⚠️
...lanner/standalone/api/OtpServerRequestContext.java 0.00% 4 Missing ⚠️
...pis/transmodel/TransmodelGraphQLSchemaFactory.java 0.00% 3 Missing ⚠️
...a/service/DefaultViaCoordinateTransferFactory.java 0.00% 3 Missing ⚠️
...lanner/ext/parkAndRideApi/ParkAndRideResource.java 0.00% 2 Missing ⚠️
...entripplanner/apis/gtfs/GraphQLRequestContext.java 0.00% 2 Missing ⚠️
...pplanner/apis/gtfs/datafetchers/QueryTypeImpl.java 50.00% 2 Missing ⚠️
.../opentripplanner/ext/ojp/resource/OjpResource.java 0.00% 1 Missing ⚠️
...pentripplanner/ext/ojp/resource/TriasResource.java 0.00% 1 Missing ⚠️
... and 2 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             dev-2.x    #7565   +/-   ##
==========================================
  Coverage      72.55%   72.55%           
- Complexity     21270    21271    +1     
==========================================
  Files           2379     2377    -2     
  Lines          86392    86424   +32     
  Branches        8487     8487           
==========================================
+ Hits           62678    62702   +24     
- Misses         20716    20724    +8     
  Partials        2998     2998

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@optionsome optionsome force-pushed the move-nearby-stop-finder branch 2 times, most recently from 1a7c15d to ab4b283 Compare May 5, 2026 09:31
@leonardehrenfried
Copy link
Copy Markdown
Member

leonardehrenfried commented May 6, 2026

Is this ready for review?

@optionsome
Copy link
Copy Markdown
Member Author

optionsome commented May 6, 2026

Is this ready for review?

Not yet.

@optionsome optionsome force-pushed the move-nearby-stop-finder branch from 2818881 to 431f087 Compare May 7, 2026 07:40
@optionsome
Copy link
Copy Markdown
Member Author

optionsome commented May 7, 2026

Now this is ready for review. I plan to create a follow up pr where I move the finders to dagger context and do some necessary refactoring to how the nearbystopfinders are constructed.

@optionsome optionsome changed the title Move nearby stop finder classes away from graph builder Move nearby stop and place finder classes to a new package and move nearby stop finding to happen in one interface May 7, 2026
leonardehrenfried
leonardehrenfried reviewed May 7, 2026
View reviewed changes
Comment thread ...main/java/org/opentripplanner/place/nearbystopfinder/PatternConsideringNearbyStopFinder.java
Comment on lines +53 to +57
public List<NearbyStop> findNearbyStops(Coordinate coordinate, double radiusMeters) {
throw new UnsupportedOperationException(
"This method is currently unsupported for this implementation."
);
}
Copy link
Copy Markdown
Member

@leonardehrenfried leonardehrenfried May 7, 2026

Choose a reason for hiding this comment

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

I cannot rememeber the conclusion: will this be split into two interfaces?

Copy link
Copy Markdown
Member Author

@optionsome optionsome May 8, 2026

Choose a reason for hiding this comment

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

We sort of decided to use one interface, but at that point we didn't really consider this implementation. I started to wonder if we actually should split the interface into two.

leonardehrenfried
leonardehrenfried previously approved these changes May 7, 2026
View reviewed changes
Copy link
Copy Markdown
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.

This is in line with what we discussed: it cleans up this package mess and moves the code into the right direction.

There will probably be follow ups but this is ready to merge.

@leonardehrenfried leonardehrenfried self-assigned this May 7, 2026
@leonardehrenfried
Copy link
Copy Markdown
Member

leonardehrenfried commented May 8, 2026

Conflicts, I'm afraid.

@optionsome optionsome requested review from leonardehrenfried and removed request for leonardehrenfried May 12, 2026 07:56
@leonardehrenfried
Copy link
Copy Markdown
Member

leonardehrenfried commented May 12, 2026 •
edited
Loading

More conflicts.

@optionsome
Copy link
Copy Markdown
Member Author

optionsome commented May 12, 2026

I think I'll squash merge this once this is approved since this now has 5+ merge commits and 1 real commit :D

@t2gran
Copy link
Copy Markdown
Member

t2gran commented May 12, 2026 •
edited
Loading

Squash : You allready lost history on one file, squashing would maybe loose history on more files. If you reset --hard to dev-2.x(de7bb53) and cherry-pick these two commits no history is lost:

The two commits is based on the same dev-2.x commit you merged in last and the result is 100% equal to to the content in this PR right now.

I will approve ✅ as soon as this is done.

@optionsome optionsome force-pushed the move-nearby-stop-finder branch from 490999a to 2b031b9 Compare May 12, 2026 19:40
@optionsome optionsome added this pull request to the merge queue May 12, 2026
Merged via the queue into opentripplanner:dev-2.x with commit 2b6a489 May 12, 2026
8 checks passed
@optionsome optionsome deleted the move-nearby-stop-finder branch May 12, 2026 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t2gran t2gran t2gran approved these changes

@leonardehrenfried leonardehrenfried Awaiting requested review from leonardehrenfried

Assignees

@leonardehrenfried leonardehrenfried

@t2gran t2gran

Labels

+Skip Changelog This is not a relevant change for a product owner since last release. !Technical Debt Improve code quality, no functional changes.

Projects

None yet

Milestone

2.10 (next release)

Development

Successfully merging this pull request may close these issues.

3 participants

@optionsome @leonardehrenfried @t2gran