Skip to content

Refactor NearbyStop to have a stopId rather than a stop#7463

Open
optionsome wants to merge 5 commits intoopentripplanner:dev-2.xfrom
HSLdevcom:nearby-stop-refactor
Open

Refactor NearbyStop to have a stopId rather than a stop#7463
optionsome wants to merge 5 commits intoopentripplanner:dev-2.xfrom
HSLdevcom:nearby-stop-refactor

Conversation

@optionsome
Copy link
Copy Markdown
Member

@optionsome optionsome commented Mar 25, 2026

Summary

Goal of this pr is to get rid of stop dependency in the NearbyStop class. There is some related refactoring to achieve this.

Issue

No issue

Unit tests

Updated

Documentation

No

Changelog

Skipped

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

codecov bot commented Mar 25, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 67.54967% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.99%. Comparing base (a50cb61) to head (a8bd24c).
⚠️ Report is 1 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...ipplanner/ext/flex/trip/ScheduledDeviatedTrip.java 16.66% 8 Missing and 2 partials ⚠️
...opentripplanner/ext/flex/trip/UnscheduledTrip.java 33.33% 8 Missing ⚠️
...r/ext/transferanalyzer/DirectTransferAnalyzer.java 0.00% 7 Missing ⚠️
...entripplanner/apis/gtfs/datafetchers/StopImpl.java 0.00% 4 Missing ⚠️
...apis/transmodel/model/stop/QuayAtDistanceType.java 20.00% 4 Missing ⚠️
...a/service/DefaultViaCoordinateTransferFactory.java 0.00% 4 Missing ⚠️
...ner/apis/gtfs/datafetchers/stopAtDistanceImpl.java 25.00% 3 Missing ⚠️
...pplanner/apis/gtfs/datafetchers/QueryTypeImpl.java 0.00% 2 Missing ⚠️
...dule/transfer/filter/FlexTripNearbyStopFilter.java 50.00% 1 Missing and 1 partial ⚠️
...t/java/org/opentripplanner/ext/flex/FlexIndex.java 75.00% 1 Missing ⚠️
... and 4 more
Additional details and impacted files 
@@              Coverage Diff              @@
##             dev-2.x    #7463      +/-   ##
=============================================
- Coverage      71.02%   70.99%   -0.03%     
+ Complexity     20995    20993       -2     
=============================================
  Files           2348     2348              
  Lines          87252    87276      +24     
  Branches        8640     8635       -5     
=============================================
- Hits           61967    61961       -6     
- Misses         22291    22320      +29     
- Partials        2994     2995       +1

☔ 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.

@jessicaKoehnke jessicaKoehnke self-assigned this Mar 26, 2026
jessicaKoehnke
jessicaKoehnke requested changes Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@jessicaKoehnke jessicaKoehnke left a comment

Choose a reason for hiding this comment

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

Nice! As a general question: Do we like using static methods, or do we rather want to have initialized components that could be injected by dagger and for example mocked in tests?

@optionsome optionsome mentioned this pull request Mar 29, 2026
Open
jessicaKoehnke
jessicaKoehnke previously approved these changes Mar 31, 2026
View reviewed changes
habrahamsson-skanetrafiken
habrahamsson-skanetrafiken reviewed Apr 1, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken 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 nice. Just one question

...src/main/java/org/opentripplanner/graph_builder/module/transfer/DirectTransferGenerator.java
// Skip the origin stop, loop transfers are not needed.
if (sd.stop == stop) {
var nearbyStop = repository.getStopLocation(sd.stopId);
if (nearbyStop == stop) {
Copy link
Copy Markdown
Contributor

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken Apr 1, 2026

Choose a reason for hiding this comment

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

Do you know if there's a reason that we use == here instead of equals?

Copy link
Copy Markdown
Member Author

@optionsome optionsome Apr 2, 2026

Choose a reason for hiding this comment

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

Stops are probably deduplicated so we shouldn't have two instances of the same stop. Although, I haven't confirmed this so not sure if I should add a comment that states that or not?

Copy link
Copy Markdown
Contributor

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken Apr 7, 2026

Choose a reason for hiding this comment

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

I think we should just switch to .equals unless there's a reason not to. Just so people don't have to wonder. But you don't have to do that in this PR unless you want to

@habrahamsson-skanetrafiken
Copy link
Copy Markdown
Contributor

habrahamsson-skanetrafiken commented Apr 7, 2026

There is a conflict

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken removed their request for review April 8, 2026 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@habrahamsson-skanetrafiken habrahamsson-skanetrafiken habrahamsson-skanetrafiken left review comments

@jessicaKoehnke jessicaKoehnke jessicaKoehnke approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees

@jessicaKoehnke jessicaKoehnke

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 @habrahamsson-skanetrafiken @jessicaKoehnke