Skip to content
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

Allow multiple feed entity types in a single feed #7

Merged
merged 5 commits into from Aug 17, 2015

Conversation

egorich239
Copy link
Contributor

It has been discussed several times in the mailing list [1][2] that the limitation on single feed entity type per feed is artificial and can be removed.

I propose to reflect this in the GTFS RT Specification.

[1] https://groups.google.com/d/msg/gtfs-realtime/_HtNTGp5LxM/VvDLkV4wTnAJ
[2] https://groups.google.com/forum/#!topic/gtfs-realtime/VCKRVEWZbUE

@@ -34,9 +34,7 @@ package transit_realtime;
//
// A feed depends on some external configuration:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole paragraph/list reads strangely; perhaps it should just be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we should probably drop this paragraph.

@skinkie
Copy link
Contributor

skinkie commented Jul 29, 2015

Should we add a tool for this change that can convert merged into unmerged and back?

@egorich239
Copy link
Contributor Author

@skinkie I don't think it belongs to the specification. On top of that, such a tool is on one side trivial, but on the other side, implementation in every language supported by, say, gtfs-realtime-bindings takes a significant effort.
Having said that, if you choose to publish such a tool in a language of your choice somewhere (say, at github.com/skinkie), I will happily refer to it somewhere in the related documentation chapters.

@skinkie
Copy link
Contributor

skinkie commented Jul 29, 2015

The more I think about such too the more problems I have with the wording of the proposal. I want to know exactly: would it be allowed to have one single entity containing all three messages within that same entity? That is going be a nightmare. This is exactly why I think a proposal should contain an implementation.

Question: should a single entity contain just one type? Where a feed may contain multiple entities of different types.

@egorich239
Copy link
Contributor Author

@skinkie It is currently disallowed to have more than one kind of sub-message in single entity. This proposal does not change that restriction: https://github.com/google/transit/blob/egorich239-multifeeds/gtfs-realtime/proto/gtfs-realtime.proto#L95-96

@egorich239
Copy link
Contributor Author

@skinkie It is also possible with recent proto2 to express such oneof restriction in the proto language itself (https://developers.google.com/protocol-buffers/docs/proto#oneof-features) however this may lack proto-compilers support for some languages, and be not compile-backward-compatible for the others. Thus while I'd personally love to see syntactic oneof happen, I haven't explored whether it implies non-zero migration cost for consumers, and would for now stay with https://github.com/google/transit/blob/egorich239-multifeeds/gtfs-realtime/proto/gtfs-realtime.proto#L95-96

@skinkie
Copy link
Contributor

skinkie commented Jul 29, 2015

I guess that would make a nice second pull request.

@egorich239
Copy link
Contributor Author

@skinkie Probably. Really depends, on proto-compilers support for https://github.com/google/gtfs-realtime-bindings#supported-languages

skinkie added a commit to bliksemlabs/gtfsrt-examples that referenced this pull request Jul 29, 2015
@skinkie
Copy link
Contributor

skinkie commented Jul 29, 2015

@egorich239
Copy link
Contributor Author

@skinkie For non-incremental, yes.

For incremental (oh god, let's not deep dive into this topic in this PR), deleted should probably go into all split feeds, because we don't know what kind of entity it had been before that.

@barbeau
Copy link
Collaborator

barbeau commented Jul 29, 2015

+1

skinkie added a commit to bliksemlabs/gtfsrt-examples that referenced this pull request Jul 29, 2015
@skinkie
Copy link
Contributor

skinkie commented Jul 29, 2015

@egorich239 fixed that for you.

Ivan Egorov added 3 commits July 29, 2015 21:56
It is clear from the previous sentences that GTFS RT refers to a GTFS feed.
Polling frequency is one of trivial implementation details, not worth mentioning.
@egorich239
Copy link
Contributor Author

@magdalar Removed the paragraphs about external configuration.
@skinkie Added link to your script to feed-entities.md

@abyrd
Copy link

abyrd commented Jul 29, 2015

I am in favor of the pull request as currently worded. I do find the requirement to create a separate feed per entity type to be somewhat artificial, and agree that it should be removed.

@magdalar
Copy link
Contributor

LGTM

@egorich239
Copy link
Contributor Author

I want to call a vote. I think I've addressed the above mentioned concerns, and the last week should be enough for the discussions on the topic. The vote will end on 13.08.2015 23:59:59 UTC.

Quick reminder on the voting rules (from https://github.com/google/transit/blob/master/gtfs-realtime/CHANGES.md)

  • During voting period only editorial changes to the proposal are allowed (typos, wording may change as long as it does not change the meaning).
  • Anyone is allowed to vote yes/no in a form of comment to the pull request, and votes can be changed until the end of the voting period. If a voter changes her vote, it is recommended to do it by updating the original vote comment by striking through the vote and writing the new vote.
    Votes before the start of the voting period are not considered.

@skinkie
Copy link
Contributor

skinkie commented Aug 5, 2015

Yes.

@magdalar
Copy link
Contributor

magdalar commented Aug 5, 2015

Yes

@abyrd
Copy link

abyrd commented Aug 5, 2015

Yes.

@barbeau
Copy link
Collaborator

barbeau commented Aug 5, 2015

Yes

egorich239 pushed a commit that referenced this pull request Aug 17, 2015
Allow multiple feed entity types in a single feed
@egorich239 egorich239 merged commit f8a8716 into master Aug 17, 2015
@egorich239 egorich239 deleted the egorich239-multifeeds branch August 17, 2015 08:13
@emmambd emmambd added the GTFS Realtime Issues and Pull Requests that focus on GTFS Realtime label May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GTFS Realtime Issues and Pull Requests that focus on GTFS Realtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants