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
Proposal: Add MODIFIED and pickup and drop-off types to GTFS-RT; experimentally deprecate SKIPPED #265
Proposal: Add MODIFIED and pickup and drop-off types to GTFS-RT; experimentally deprecate SKIPPED #265
Changes from 8 commits
2faa3df
b28420b
898c1a3
9a57863
bf8bfa7
591ddd8
16e412f
c475594
89993cc
19fc85f
107fb2a
780be36
e9ef94b
ab21cd6
81c94d7
4ffaf3a
d28f432
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.
It looks like you're effectively deprecating
SKIPPED
? If so, please mark it as deprecated in the .proto with[deprecated=true]
and docs.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.
Originally, I was thinking that the spec would continue to support
SKIPPED
for service that's fully missed, such as due to a stop closure. That said, there is ambiguity in that scenario in terms of whether or not you would doSKIPPED
orMODIFIED
withpickup_type=NO_PICKUP
anddrop_off_type=NO_DROP_OFF
.Do you have a suggestion in terms of whether or not deprecation makes sense or instead to specify that producers should not use both
NO_PICKUP
andNO_DROP_OFF
at the same time and continue to useSKIPPED
?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.
SKIPPED
is currently defined as:Without additional context, I think
SKIPPED
andpickup_type=NO_PICKUP
withdrop_off_type=NO_DROP_OFF
are effectively the same case from a consumers perspective (but correct me if I'm wrong). If that's true, I'd argue that we should deprecateSKIPPED
in favor of additional granularity inpickup_type
anddrop_off_type
so there is one clear way of representing this information going forward.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.
Unless I'm misunderstanding, deprecating
SKIPPED
require producers and consumers to stop using it and instead usepickup_type=NO_PICKUP
withdrop_off_type=NO_DROP_OFF
. Eric's suggestion "to specify that producers should not use bothNO_PICKUP
andNO_DROP_OFF
at the same time and continue to useSKIPPED
" seems like a better approach.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.
Not immediately - the long-term plan would be to stop using
SKIPPED
(i.e., it's deprecated, signaling that at some point it will be removed entirely from the spec and .proto), but there needs to be a short term migration plan (like Eric's suggestion to use both in parallel for some time) because it will take consumers and producers time to switch over. Using[deprecated=true]
in the .proto allows producers and consumers to still continue using that field in the bindings software for now, it will just mark the relevant functions as deprecated.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.
Deprecating
SKIPPED
and using the proposed migration plan sounds good in theory, but we do have concerns about this in practice. There are a lot of producers and consumers who would need to make updates to stop usingSKIPPED
, and we could see the[deprecated=true]
sticking around for a very long time. Would there be a point at which the field would be removed altogether?Another practical concern is that the migration plan makes suggestions instead of requirements.
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.
Completely agree here about the practical considerations of deprecating fields. I think it's fair to say that if we were designing the spec from scratch, we probably wouldn't have both
SKIPPED
andNO_PICKUP
andNO_DROP_OFF
to avoid the ambiguity of representing the same scenario two different ways. Even when documented, if it's technically allowed, it creates room for confusion.I would expect it to take quite a while for both consumers and producers to update (perhaps years), but I think signally it now by marking it explicitly as deprecated allows us the ability to eventually clean up the ambiguity.