Skip to content

Small clean up of flex duration factor/offset - #5856

Merged
leonardehrenfried merged 4 commits into
opentripplanner:dev-2.xfrom
ibi-group:flex-duration-factors
Jun 3, 2024
Merged

Small clean up of flex duration factor/offset#5856
leonardehrenfried merged 4 commits into
opentripplanner:dev-2.xfrom
ibi-group:flex-duration-factors

Conversation

@leonardehrenfried

Copy link
Copy Markdown
Member

Summary

After merging of #5796 I realised that I missed a couple of renames where I still used the old name for time penalty (modifier, factors). This is fixed in this PR.

I also cleaned up the modifcation logic a bit: the check for isZero is no longer necessary as the default is a value that doesn't modify at all.

Lastly, I added a test that makes sure that the mapper sets the correct default value.

@leonardehrenfried
leonardehrenfried requested a review from a team as a code owner May 19, 2024 07:24
@codecov

codecov Bot commented May 19, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.37%. Comparing base (18d742f) to head (3faf591).
Report is 22 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5856      +/-   ##
=============================================
+ Coverage      67.95%   68.37%   +0.42%     
- Complexity     16607    16677      +70     
=============================================
  Files           1914     1914              
  Lines          72648    72650       +2     
  Branches        7452     7452              
=============================================
+ Hits           49368    49675     +307     
+ Misses         20750    20418     -332     
- Partials        2530     2557      +27     

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

@leonardehrenfried leonardehrenfried 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 May 19, 2024
Comment thread src/test/java/org/opentripplanner/gtfs/mapping/TripMapperTest.java Outdated
Comment thread src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java Outdated
optionsome
optionsome previously approved these changes May 22, 2024
@leonardehrenfried
leonardehrenfried requested a review from t2gran May 23, 2024 14:03
@Nullable
@Override
public String logName() {
return "timePenalty=(%s)".formatted(timePenalty.toString());

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.

I do not think this is what the logName() method is intended for - it is to add name/description to entity logging. If the goal is to tests the penalty, the best way to do it is to test that the calculator retuned is of type TimePenaltyCalculator - that should be suffisient, since I assume there is unit tests on the calculator - no need to test the same features twice. Theoretical this is a white-box testing, but it is better than nothing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I feel a bit stupid because I should have thought of that myself. 🤦‍♂️

@leonardehrenfried
leonardehrenfried merged commit c71c2a7 into opentripplanner:dev-2.x Jun 3, 2024
@leonardehrenfried
leonardehrenfried deleted the flex-duration-factors branch June 3, 2024 10:32
@t2gran t2gran added this to the 2.6 (next release) milestone Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

3 participants