Skip to content

Empty stop_headsign will fall back to trip_headsign#5205

Merged
leonardehrenfried merged 10 commits into
opentripplanner:dev-2.xfrom
HBTGmbH:fallback-stop-headsigns
Jun 29, 2023
Merged

Empty stop_headsign will fall back to trip_headsign#5205
leonardehrenfried merged 10 commits into
opentripplanner:dev-2.xfrom
HBTGmbH:fallback-stop-headsigns

Conversation

@2martens

Copy link
Copy Markdown
Contributor

Summary

If a stop_headsign is not set, OTP will fall back to trip_headsign.

Issue

#5204

Closes #5204

Unit tests

Added test that covers all scenarios:

trip_headsign stop_headsign result
null null null
DIRECTION null DIRECTION
null DIRECTION DIRECTION
DIRECTION DIRECTION DIRECTION
DIRECTION STOP STOP

Additionally verified the solution manually with our data.

@2martens
2martens requested a review from a team as a code owner June 20, 2023 13:18
@codecov

codecov Bot commented Jun 20, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.15 🎉

Comparison is base (b228933) 65.39% compared to head (f7d7918) 65.54%.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5205      +/-   ##
=============================================
+ Coverage      65.39%   65.54%   +0.15%     
- Complexity     14476    14614     +138     
=============================================
  Files           1755     1761       +6     
  Lines          68005    68343     +338     
  Branches        7230     7289      +59     
=============================================
+ Hits           44474    44798     +324     
+ Misses         21067    21055      -12     
- Partials        2464     2490      +26     
Impacted Files Coverage Δ
...tripplanner/transit/model/timetable/TripTimes.java 81.88% <100.00%> (+0.13%) ⬆️

... and 67 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@leonardehrenfried leonardehrenfried added !Bug Apply to issues describing a bug and PRs witch fixes it. +GTFS Related to import of GTFS data labels Jun 20, 2023
@leonardehrenfried

Copy link
Copy Markdown
Member

Can you autoformat your code?

@2martens

Copy link
Copy Markdown
Contributor Author

I had already done it. But apparently there are still things missing.

@leonardehrenfried
leonardehrenfried self-requested a review June 22, 2023 13:34
import org.opentripplanner.transit.model._data.TransitModelForTest;
import org.opentripplanner.transit.model.framework.Deduplicator;

class TripTimesTest {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We already have a TripTimesTest but it's in the wrong package.

Can you move it here and rename it TripTimesUpdateTest?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

With a bit of refactoring, it was actually quite easy to merge both test classes.

@leonardehrenfried leonardehrenfried Jun 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for that.

Could I perhaps ask for one more round of refactoring?

  • remove the @BeforeAll from createInitialTripTimes, simply call the method in those tests that actually need it and make originalTripTimes a local variable
  • move your headsign tests into a nested class with the @nested annotation, like this

I know it's a bit nitpicky but I like well structured tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will look into it.

Comment thread src/test/java/org/opentripplanner/transit/model/timetable/TripTimesTest.java Outdated
@leonardehrenfried

Copy link
Copy Markdown
Member

@hbtjma I'm assuming that you don't have the merge permission so I will take the liberty to do it for you.

@leonardehrenfried
leonardehrenfried merged commit a377450 into opentripplanner:dev-2.x Jun 29, 2023
t2gran pushed a commit that referenced this pull request Jun 29, 2023
@2martens
2martens deleted the fallback-stop-headsigns branch June 29, 2023 14:14
@t2gran t2gran added this to the 2.4 (next release) milestone Jul 18, 2023
@leonardehrenfried

leonardehrenfried commented Aug 15, 2023

Copy link
Copy Markdown
Member

Related: #5290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it. +GTFS Related to import of GTFS data

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OTP does not fall back to trip_headsign if stop_headsign is empty/not set

4 participants