Skip to content

Fix itinerary cost review #6539

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

Merged
merged 4 commits into from
Mar 20, 2025

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Mar 14, 2025

Summary

Small fixes suggest in the review of #6535.

Renaming ItineraryFares to ItineraryFares is debatable:

  • In old Java conventions plural is used for naming variables and classes witch are of collection type, not necesseraly extending the java collections, but something containing a collection of the singular form.
  • Note that ItineraryFare is related to the itinerary, it does contain a collection of fares for legs - but only one fare for the total Itinerary.

Alternative name: ItineraryFareInfo

Issue

🟥 This is code cleanup

Unit tests

🟥 No new functional changes

Documentation

✅ JavaDoc is updated

Changelog

🟥 This is code cleanup

Bumping the serialization version id

🟥 No changes to serialized classes

Copy link

codecov bot commented Mar 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.23%. Comparing base (52dc65f) to head (fa6efcb).
Report is 85 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6539      +/-   ##
=============================================
- Coverage      70.24%   70.23%   -0.01%     
+ Complexity     18369    18362       -7     
=============================================
  Files           2087     2087              
  Lines          77381    77377       -4     
  Branches        7839     7839              
=============================================
- Hits           54357    54347      -10     
- Misses         20249    20252       +3     
- Partials        2775     2778       +3     

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

@leonardehrenfried
Copy link
Member

You say that it doesn't contain several fares for the itinerary which is not true:

private final Set<FareProduct> itineraryProducts = new LinkedHashSet<>();

@leonardehrenfried
Copy link
Member

leonardehrenfried commented Mar 14, 2025

Can you cherry-pick (not merge) this commit to re-enable the fares tests: leonardehrenfried@61eab43

@t2gran
Copy link
Member Author

t2gran commented Mar 18, 2025

You say that it doesn't contain several fares for the itinerary which is not true

Correct me if I am wrong! But, it is relative to the itinerary - it contains many fars for LEGS, but only one option for the itinerary - it is not a collection of itinerary fares - as the name literally suggest.

@leonardehrenfried
Copy link
Member

leonardehrenfried commented Mar 18, 2025

There can also be several options for the entire itinerary. For example you can buy a weekly or a monthly ticket, which both cover the entire itinerary.

@t2gran
Copy link
Member Author

t2gran commented Mar 18, 2025

Can you cherry-pick (not merge) this commit to re-enable the fares tests: leonardehrenfried@61eab43

I already did that in the original PR: f46d68a

@leonardehrenfried
Copy link
Member

Ah, sorry, I missed that.

@t2gran
Copy link
Member Author

t2gran commented Mar 18, 2025

There can also be several options for the entire itinerary. For example you can buy a weekly or a monthly ticket, which both cover the entire itinerary.

Ok, I kind of suspected that could be the case, but it is still not a collection of "itinerary fares". I can agree this is a boarder case - but the key thing here is OOP thinking - and not about correct english.

@leonardehrenfried
Copy link
Member

There can also be several options for the entire itinerary. For example you can buy a weekly or a monthly ticket, which both cover the entire itinerary.

Ok, I kind of suspected that could be the case, but it is still not a collection of "itinerary fares". I can agree this is a boarder case - but the key thing here is OOP thinking - and not about correct english.

To be honest, I don't really care all that much what it is called and will follow your suggestion.

@t2gran t2gran merged commit 8728818 into opentripplanner:dev-2.x Mar 20, 2025
6 checks passed
@t2gran t2gran deleted the fix_itinerary_cost_review branch March 20, 2025 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants