Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: False positives for StopTimeTimepointWithoutTimesNotice #1044
fix: False positives for StopTimeTimepointWithoutTimesNotice #1044
Changes from 14 commits
a72b888
febb426
c2fa449
b3ec654
fc14880
d44332a
fa647ef
8e91071
c1d8fc3
7497e16
bab59bf
df1f702
a2b5e3b
ddf420b
1135049
539fc1f
2c62b0a
62c64c1
afa44dc
4420e7d
b9dd33f
22c59aa
e33c960
dc6462c
0a74e40
84926f4
35f47ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 :)
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.
usage of
stopTime.hasTimepoint()
shouldn't be necessary, becausetimepoint
has a default value (see [0]). Was it added because unit tests don't respect the default values?[0]
gtfs-validator/main/src/main/java/org/mobilitydata/gtfsvalidator/table/GtfsStopTimeSchema.java
Line 77 in 5243445
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.
Actually because of the fact that
timepoint
has a default value we need to checkstopTime.hasTimepoint()
. See, if astopTime
has no value for timepoint or has value1
fortimepoint
,stopTime.timepoint()
returns the same valueEXACT
. UsingstopTime.hasTimepoint()
makes it possible to distinguish these two situations.Bottom line, a
stopTime
is considered atimepoint
if the value is provided and set to1
, otherwise it is not.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.
I see, in this case should we remove default value instead?
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.
That would make sense, thoughts @isabelle-dr @barbeau ?
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.
Agreed to remove default value.
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.
@asvechnikov2 I was trying to trace the execution for this too. Where do you see this assignment?
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.
I think it's this one
gtfs-validator/processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/EntityImplementationGenerator.java
Lines 134 to 160 in 903e9af
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.
@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
GtfsStopTime.DEFAULT_TIMEPOINT
variable value.So the current behavior is:
@DefaultValue
annotation, if the column is missing or the row has an empty value for an enum then the default value of0
is used.@DefaultValue
annotation, then the provided default value is used.So the default enum value is the value defined for
0
- forGtfsStopTimeTimepoint
this isAPPROXIMATE(0)
. This is similar to protocol buffer bindings behavior. I agree that explicitly defining the default enum value everywhere via@DefaultValue
would be a good best practice to avoid confusion, especially for newcomers adding new fields where is isn't obvious that the0
enum value is the de facto default.Back to the original question - if we remove
@DefaultValue("1")
, the new value oftimepoint
field would beGtfsStopTimeTimepoint.APPROXIMATE
if the timepoint column is missing or the field has an empty value in the CSV. We don't want this either because of legacy datasets from the original GTFS spec before the timepoint field existed (because all those records with times should be consideredEXACT
).IMHO the best solution is still having the default is set to
EXACT
IFF the timepoint column isn't provided (EDIT - we also have to consider the presence/absence of times - if times are provided it'sEXACT
, otherwiseAPPROXIMATE
), and otherwise have the default set toUNRECOGNIZED
for empty. I think the 2nd best solution is to leave the@DefaultValue("1")
, because that aligns with the legacy GTFS datasets and possible interpretations of the current GTFS spec.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.
@barbeau, @asvechnikov2
After discussion with @isabelle-dr, we agreed on keeping the
@DefaultValue("1")
for now since removing this entails deeper conversations about how default values should be dealt with. We suggest to keep track of this in a separate issue and work on it as future improvement.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd add a comment to clarify the behaviour, here and probably in the class docstring as well.