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
Deprecate TripUpdate.schedule_relationship = ADDED, add TripUpdate.schedule_relationship = NEW / REPLACEMENT to specify new / replaced trips which do not run on a schedule from the GTFS static. #504
base: master
Are you sure you want to change the base?
Deprecate TripUpdate.schedule_relationship = ADDED, add TripUpdate.schedule_relationship = NEW / REPLACEMENT to specify new / replaced trips which do not run on a schedule from the GTFS static. #504
Changes from all commits
dc181de
b439cf8
5a9853f
21ccde5
b6d07cc
6bc14c8
87d28c7
991caac
fea9f81
b6faa23
0e62760
896129e
43a9b00
e22668e
3ff34bf
573124c
ade68ae
2f019d8
555a619
a2a0dd8
aa90291
4361f12
d0843d5
7804def
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.
I don't like this approach, there for I think it's good to seriously consider a version bump where the 'ADDED' enumeration is completely removed as @skinkie suggested.
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'm sorry, but as Google mentioned, we can't break backward compatibility.
A decade ago, when REPLACEMENT was removed from the spec, there was a version of .proto file without that value published, and it horribly broke consumers on existing feeds when they were compiled with an updated .proto . Therefore the value has been put back and marked as deprecated.
Therefore the value ADDED has to remain even if we don't want to use it anymore, and those consumers who are using ADDED for the purpose I am migrating to NEW (like OpenTripPlanner) can treat it the same as NEW 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.
We don't break backwards compatibility when bumping a version.
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.
@sven4all @skinkie
May I confirm what you expect for versioning here? (I saw option A in the beginning of this conversation, just to double check...)
A. Bump version number, add "NEW" and completely deprecate "ADDED"
B. Bump version number, add "NEW", "ADDED" coexist (with transition guide)
C. Bump version number, no "NEW" but make changes on "ADDED"
Perhaps some of these look acceptable or ideal for 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.
I am preferring C. I absolutely don't want a situation which is suggested by B where a bumped version exists, and data ends up in both NEW and ADDED under that higher version.
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.
Let me be up front that if we are considering a version bump to GTFS-Realtime (either major or minor), then I don't think we'd do it for just this proposal. We've never done a version bump of GTFS-Realtime, so this would need a broader discussion of whether we'd actually do it, what the process looks like, expectations around support for old and new versions, and if we come to terms with all that, then an extended phase where we discuss any other breaking-changes that we'd want to roll into the new version (think something akin to the GBFS process).
Which is to say, I don't want you to be surprised if/when I vote -1 on any single proposal here with a version bump.
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.
The GTFS-realtime version is now 2.0, where it was 1.0 in the past, so it has been done already.
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.
True, but my concern still stands.
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.
Also, when REPLACEMENT was removed a decade ago (because it was never fully specified, like ADDED right now), breaking changes to the .proto was actually released into the wild as well.
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 wouldn't consider 2.0 a real breaking change. In practice, it only formalized that some states were invalid. In the past, those invalid state were valid per the specification but no one really considered those state valid. #64
In other words, I don't think anyone has special code handling gtfs-rt 1.0 and 2.0.