fix: False positives for StopTimeTimepointWithoutTimesNotice #1044
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,11 +16,13 @@ | |
|
|
||
| package org.mobilitydata.gtfsvalidator.validator; | ||
|
|
||
| import javax.inject.Inject; | ||
| import org.mobilitydata.gtfsvalidator.annotation.GtfsValidator; | ||
| import org.mobilitydata.gtfsvalidator.notice.NoticeContainer; | ||
| import org.mobilitydata.gtfsvalidator.notice.SeverityLevel; | ||
| import org.mobilitydata.gtfsvalidator.notice.ValidationNotice; | ||
| import org.mobilitydata.gtfsvalidator.table.GtfsStopTime; | ||
| import org.mobilitydata.gtfsvalidator.table.GtfsStopTimeTableContainer; | ||
| import org.mobilitydata.gtfsvalidator.table.GtfsStopTimeTableLoader; | ||
| import org.mobilitydata.gtfsvalidator.table.GtfsStopTimeTimepoint; | ||
|
|
||
|
|
@@ -32,56 +34,101 @@ | |
| * <ul> | ||
| * <li>{@link StopTimeTimepointWithoutTimesNotice} - a timepoint does not specifies arrival_time | ||
| * or departure_time | ||
| * <li>{@link MissingTimepointValueNotice} - value for {@code stop_times.timepoint} is missing | ||
| * <li>{@link MissingTimepointColumnNotice} - field {@code stop_times.timepoint} is missing | ||
| * </ul> | ||
| */ | ||
| @GtfsValidator | ||
| public class TimepointTimeValidator extends FileValidator { | ||
| private final GtfsStopTimeTableContainer stopTimes; | ||
|
|
||
| @Inject | ||
| TimepointTimeValidator(GtfsStopTimeTableContainer stopTimes) { | ||
| this.stopTimes = stopTimes; | ||
| } | ||
|
|
||
| @Override | ||
| public void validate(NoticeContainer noticeContainer) { | ||
| if (!stopTimes.hasColumn(GtfsStopTimeTableLoader.TIMEPOINT_FIELD_NAME)) { | ||
| // legacy datasets do not use timepoint column in stop_times.txt as a result: | ||
| // - this should be flagged; | ||
| // - but also no notice regarding the absence of arrival_time or departure_time should be | ||
| // generated | ||
| noticeContainer.addValidationNotice(new MissingTimepointColumnNotice()); | ||
| return; | ||
| } | ||
| for (GtfsStopTime stopTime : stopTimes.getEntities()) { | ||
| if (!stopTime.hasTimepoint()) { | ||
| noticeContainer.addValidationNotice(new MissingTimepointValueNotice(stopTime)); | ||
| } | ||
| if (isTimepoint(stopTime)) { | ||
| if (!stopTime.hasArrivalTime()) { | ||
| noticeContainer.addValidationNotice( | ||
| new StopTimeTimepointWithoutTimesNotice( | ||
| stopTime, GtfsStopTimeTableLoader.ARRIVAL_TIME_FIELD_NAME)); | ||
| } | ||
| if (!stopTime.hasDepartureTime()) { | ||
| noticeContainer.addValidationNotice( | ||
| new StopTimeTimepointWithoutTimesNotice( | ||
| stopTime, GtfsStopTimeTableLoader.DEPARTURE_TIME_FIELD_NAME)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private boolean isTimepoint(GtfsStopTime stopTime) { | ||
| return stopTime.hasTimepoint() && stopTime.timepoint().equals(GtfsStopTimeTimepoint.EXACT); | ||
|
There was a problem hiding this comment. usage of [0] There was a problem hiding this comment. Actually because of the fact that Bottom line, a There was a problem hiding this comment. I see, in this case should we remove default value instead? There was a problem hiding this comment. That would make sense, thoughts @isabelle-dr @barbeau ? There was a problem hiding this comment. Agreed to remove default value. There was a problem hiding this comment.
@asvechnikov2 I was trying to trace the execution for this too. Where do you see this assignment? There was a problem hiding this comment. I think it's this one There was a problem hiding this comment. @asvechnikov2 Ah, thanks! Looks like you're right. I just ran this through the debugger with two different datasets to confirm: This code is what auto-generates the So the current behavior is:
So the default enum value is the value defined for Back to the original question - if we remove IMHO the best solution is still having the default is set to There was a problem hiding this comment.
After discussion with @isabelle-dr, we agreed on keeping the If you agree with that, I'll ask one of you their approval on this PR if the code looks good to you :) There was a problem hiding this comment. nit: I'd add a comment to clarify the behaviour, here and probably in the class docstring as well. |
||
| } | ||
|
|
||
| /** | ||
| * Timepoint without time | ||
| * | ||
| * <p>Severity: {@code SeverityLevel.WARNING} - to be upgraded as {@code SeverityLevel.ERROR} | ||
lionel-nj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| */ | ||
| static class StopTimeTimepointWithoutTimesNotice extends ValidationNotice { | ||
| private final long csvRowNumber; | ||
| private final String tripId; | ||
| private final long stopSequence; | ||
| private final String specifiedField; | ||
|
|
||
| StopTimeTimepointWithoutTimesNotice(GtfsStopTime stopTime, String specifiedField) { | ||
| super(SeverityLevel.WARNING); | ||
| this.csvRowNumber = stopTime.csvRowNumber(); | ||
| this.tripId = stopTime.tripId(); | ||
| this.stopSequence = stopTime.stopSequence(); | ||
| this.specifiedField = specifiedField; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * {@code stop_times.timepoint} value is missing | ||
| * | ||
| * <p>Severity: {@code SeverityLevel.WARNING} | ||
| */ | ||
| static class MissingTimepointValueNotice extends ValidationNotice { | ||
| private final long csvRowNumber; | ||
| private final String tripId; | ||
| private final long stopSequence; | ||
|
|
||
| MissingTimepointValueNotice(GtfsStopTime stopTime) { | ||
| super(SeverityLevel.WARNING); | ||
| this.csvRowNumber = stopTime.csvRowNumber(); | ||
| this.tripId = stopTime.tripId(); | ||
| this.stopSequence = stopTime.stopSequence(); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Column {@code stop_times.timepoint} is missing. | ||
| * | ||
| * <p>Severity: {@code SeverityLevel.WARNING} | ||
| */ | ||
| static class MissingTimepointColumnNotice extends ValidationNotice { | ||
| private final String filename; | ||
|
|
||
| MissingTimepointColumnNotice() { | ||
| super(SeverityLevel.WARNING); | ||
| this.filename = GtfsStopTimeTableLoader.FILENAME; | ||
| } | ||
| } | ||
| } | ||
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.
Specification says
1 or empty - Times are considered exact.. Are we going to change specification?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.
We will advocate for @barbeau's recommendation in google/transit#61. If you support this recommendation or have other ideas, feel free to leave a comment there so we make a PR accordingly :)