Skip to content

Itinerary cleanup #6617

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.

Sign up for GitHub

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

Jump to bottom
Merged
t2gran merged 13 commits into from
May 7, 2025
Merged

Itinerary cleanup #6617

t2gran merged 13 commits into from
May 7, 2025

Conversation

@t2gran
Copy link
Member

@t2gran t2gran commented Apr 14, 2025 •
edited
Loading

Summary

This is a pure refactoring/cleanup. I will base my next PR on top of this.

A few unit-tests and utility methods is added.

Issue

🟥 No issue on this

Unit tests

🟥 Unit-tests are unchanged

Documentation

🟥 The code/doc is not touched.

Changelog

🟥 Not relevant

Bumping the serialization version id

🟥 The changed code is not part of the serialized graph.

t2gran added 4 commits April 13, 2025 18:56
@t2gran
refactor: Inline first/last legs
36092a8
@t2gran
refactor: Rename to UnknownPathLeg from UnknownTransitPathLeg
142c3e2 
This is not a transit path, but any type - the transit part
of the name refer to where it was produced, but that should
not be part of a name.
@t2gran
refactor: Move walk-steps into its own package
e4e7424
@t2gran
refactor: Move itinerary leg classes into its own package
2884115
@t2gran t2gran 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 14, 2025
@t2gran t2gran added this to the 2.8 (next release) milestone Apr 14, 2025
@t2gran t2gran requested a review from a team as a code owner April 14, 2025 09:27
@codecov
Copy link

codecov bot commented Apr 14, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 77.75229% with 97 lines in your changes missing coverage. Please review.

Project coverage is 71.19%. Comparing base (a5a328b) to head (fcd9293).
Report is 52 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...er/apis/transmodel/model/TripTimeOnDateHelper.java 0.00% 19 Missing ⚠️
...ripplanner/model/plan/leg/FrequencyTransitLeg.java 0.00% 11 Missing ⚠️
...ripplanner/apis/transmodel/model/plan/LegType.java 71.87% 9 Missing ⚠️
...r/ext/fares/impl/CombinedInterlinedTransitLeg.java 52.94% 8 Missing ⚠️
...opconsolidation/DecorateConsolidatedStopNames.java 56.25% 2 Missing and 5 partials ⚠️
...pentripplanner/apis/gtfs/datafetchers/LegImpl.java 80.55% 7 Missing ⚠️
...opentripplanner/ext/fares/impl/HSLFareService.java 71.42% 3 Missing and 1 partial ⚠️
...java/org/opentripplanner/model/plan/Itinerary.java 81.81% 2 Missing and 2 partials ⚠️
...er/routing/algorithm/mapping/AlertToLegMapper.java 69.23% 0 Missing and 4 partials ⚠️
...g/opentripplanner/ext/flex/FlexibleTransitLeg.java 89.28% 3 Missing ⚠️
... and 15 more
Additional details and impacted files 
@@            Coverage Diff             @@
##             dev-2.x    #6617   +/-   ##
==========================================
  Coverage      71.19%   71.19%           
- Complexity     18465    18468    +3     
==========================================
  Files           2026     2026           
  Lines          76289    76278   -11     
  Branches        7807     7804    -3     
==========================================
- Hits           54312    54307    -5     
+ Misses         19218    19212    -6     
  Partials        2759     2759

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

t2gran added 5 commits April 15, 2025 09:42
@t2gran
Merge remote-tracking branch 'entur/add_co2_emissions' into add_emiss…
93de750 
…ion_to_transmodl_api

# Conflicts:
#	application/src/ext/java/org/opentripplanner/ext/emissions/itinerary/DecorateWithEmission.java
#	application/src/main/java/org/opentripplanner/apis/gtfs/generated/graphql-codegen.yml
@t2gran
refactor: Rename Leg copy() to copyOf()
b270f46
@t2gran
refactor: Use "[a, b]" as range notation, not "[a - b]"
f559dc5
@t2gran
refactor: Rename test, name after "unit" - not builder.
0e66158
@t2gran
refactor: Cleanup ScheduledTransitLeg and improve tests.
5b4c643
@t2gran t2gran marked this pull request as draft April 22, 2025 23:00
@t2gran
Copy link
Member Author

t2gran commented Apr 22, 2025 •
edited
Loading

I have merged in #6614 into this PR, so #6614 needs to be reviewed first. (This is done ✅ )

@t2gran
refactor: Refactor Itinerary::withLegs(legs -> legs)
5c711f7 
The old method was a bit awkward and is replaced with a
generic withLegs(legs) and a transformLegs(legMapper).
@t2gran t2gran marked this pull request as ready for review April 30, 2025 14:12
@t2gran
Merge remote-tracking branch 'otp/dev-2.x' into itinerary_cleanup
1bded30 
# Conflicts:
#	application/src/ext/java/org/opentripplanner/ext/fares/impl/GtfsFaresV2Service.java
leonardehrenfried
leonardehrenfried requested changes Apr 30, 2025
View reviewed changes
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.

I noticed a few typos.

optionsome
optionsome reviewed May 2, 2025
View reviewed changes
...ation/src/main/java/org/opentripplanner/apis/transmodel/mapping/RelativeDirectionMapper.java
package org.opentripplanner.apis.transmodel.mapping;

import org.opentripplanner.model.plan.RelativeDirection;
import org.opentripplanner.model.plan.walkstep.RelativeDirection;
Copy link
Member

@optionsome optionsome May 2, 2025

Choose a reason for hiding this comment

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

Should walkstep be inside the leg package since walksteps are always part of a leg?

Copy link
Member Author

@t2gran t2gran May 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I considered it, but fell down on that is is independent of leg. I do not think there is a right or wrong in this case. We can move it if you find the model more intuitive that way. The way I see it is that walk step represent the path you walk between to points in the street graph, and that a WalkLeg is the relationship between an itinerary and the walk-steps. So the walk-steps could in theory be used without an itinerary/leg.

@t2gran @leonardehrenfried @optionsome
Apply suggestions from code review
fcd9293 
Co-authored-by: Leonard Ehrenfried <[email protected]>
Co-authored-by: Joel Lappalainen <[email protected]>
@t2gran t2gran merged commit 2cc021e into opentripplanner:dev-2.x May 7, 2025
7 checks passed
@t2gran t2gran deleted the itinerary_cleanup branch May 7, 2025 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leonardehrenfried leonardehrenfried leonardehrenfried approved these changes

@optionsome optionsome optionsome approved these changes

Assignees

No one assigned

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

Development

Successfully merging this pull request may close these issues.

3 participants

@t2gran @leonardehrenfried @optionsome