-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Improve support for EPIP NeTex profile #4863
Improve support for EPIP NeTex profile #4863
Conversation
… common super class OperatingPeriod_VersionStructure (opentripplanner#3640)
…etex-profile(opentripplanner#3640) # Conflicts: # src/main/java/org/opentripplanner/datastore/file/ZipFileDataSource.java # src/main/resources/Message_de.properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a couple of questions.
if ( | ||
stopPoint != null && | ||
(isFalse(stopPoint.isForAlighting()) && isFalse(stopPoint.isForBoarding())) | ||
) { | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to skip these? They are already marked as for not for alighting/boarding in mapToStopTime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our dataset contains many of these stops as they're usually used to model tariff barriers. There are no PassengerStopAssignments for these stopPoints, so "lookUpStopLocation" yields null which results in the trip not being mapped at all.
The attempt was to ignore these for now, but I guess we should find a better solution
src/main/java/org/opentripplanner/netex/loader/parser/ServiceCalendarFrameParser.java
Show resolved
Hide resolved
src/main/java/org/opentripplanner/netex/mapping/GroupOfStationsMapper.java
Show resolved
Hide resolved
src/main/java/org/opentripplanner/netex/mapping/calendar/DayTypeAssignmentMapper.java
Outdated
Show resolved
Hide resolved
Can you run the autoformatter with |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #4863 +/- ##
=============================================
+ Coverage 62.93% 63.02% +0.08%
- Complexity 13263 13301 +38
=============================================
Files 1662 1662
Lines 66330 66376 +46
Branches 7215 7227 +12
=============================================
+ Hits 41748 41831 +83
+ Misses 22200 22168 -32
+ Partials 2382 2377 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I have only a few minor comments. Will you add NeTEx test data as part of this PR or in a separate one?
|
||
import org.opentripplanner.graph_builder.issue.api.DataImportIssue; | ||
|
||
public class InvalidTimeZone implements DataImportIssue { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now recommended to use the generic Issue builder instead of creating a custom class. See
OpenTripPlanner/src/main/java/org/opentripplanner/netex/mapping/FlexStopsMapper.java
Line 67 in 1c18194
Issue.issue( |
@@ -244,8 +244,8 @@ private void parseJourneyPatterns(JourneyPatternsInFrame_RelStructure journeyPat | |||
if (journeyPatterns == null) return; | |||
|
|||
for (JAXBElement<?> pattern : journeyPatterns.getJourneyPattern_OrJourneyPatternView()) { | |||
if (pattern.getValue() instanceof JourneyPattern) { | |||
this.journeyPatterns.add((JourneyPattern) pattern.getValue()); | |||
if (pattern.getValue() instanceof JourneyPattern_VersionStructure) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use pattern matching for instanceof (was missing in the original code but this is an opportunity to improve it)
boolean isAvailable = isDayTypeAvailableForAssigment(dayTypeAssignment); | ||
|
||
String ref = dayTypeAssignment.getOperatingPeriodRef().getRef(); | ||
OperatingPeriod period = operatingPeriods.lookup(ref); | ||
OperatingPeriod_VersionStructure period = operatingPeriods.lookup(ref); | ||
|
||
if (period != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove the null check (instanceof is false with null)
4270056
to
2751fff
Compare
Summary
This PR improves compatibility with NeTEx-dataset specified in the EU-profile.
Issue
#3640
Unit tests
Added test for parsing UicOperationPeriod in DateTypeAssignmentMapper