Check for NO_VALUE when mapping GTFS booking rules #7029
Conversation
leonardehrenfried
added
!Improvement
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #7029 +/- ##
=============================================
- Coverage 72.12% 72.11% -0.02%
- Complexity 20042 20055 +13
=============================================
Files 2175 2177 +2
Lines 80860 80929 +69
Branches 8127 8135 +8
=============================================
+ Hits 58322 58359 +37
- Misses 19672 19697 +25
- Partials 2866 2873 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
habrahamsson-skanetrafiken
requested review from
binh-dam-ibigroup and
habrahamsson-skanetrafiken
habrahamsson-skanetrafiken
left a comment
habrahamsson-skanetrafiken
left a comment
There was a problem hiding this comment.
This looks fine, just a line of documentation that needs updating. It would be nice if oba had a more explict api with optionals. Having magic numbers like this feels very fragile and scary.
application/src/test/java/org/opentripplanner/gtfs/mapping/BookingRuleMapperTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Henrik Abrahamsson <127481124+habrahamsson-skanetrafiken@users.noreply.github.com>
These magic numbers are awful but changing this would be a whole different library. If we want to change the approach of reading GTFS I think we should switch to streaming GTFS files line by line rather than reading the whole thing into memory, like I have done here: #6754 |
|
Ah, I hadn't seen that PR. It looks like a good improvement IMO. |
| rule.setPriorNoticeLastTime(0); | ||
| rule.setPriorNoticeLastDay(0); | ||
| var rule = rule("3"); | ||
| // when either of prior notice time and day are set to -999(NO_VALUE), they should be treated as "not set". |
There was a problem hiding this comment.
| // when either of prior notice time and day are set to -999(NO_VALUE), they should be treated as "not set". | |
| // when either of prior notice time and day are set to -999 (NO_VALUE), they should be treated as "not set". |
3 participants
Summary
Previously OBA used the value
0in booking rules times meaning "no value". This is problematic because 0 is also a valid value meaning either midnight of zero.OBA now uses NO_VALUE (-999) to mean "no value" and so the GTFS import had to be changed, too.
Unit tests
Added.
Documentation
Javadoc update.