public-transport / friendly-public-transport-format

A format for APIs, libraries and datasets containing and working with public transport data.
Creative Commons Attribution Share Alike 4.0 International
125 stars 1 forks source link

add basic protocol buffer implementation #20

Closed juliuste closed 7 years ago

juliuste commented 7 years ago

This does only support ID references and no "inline referencing" for FPTF objects for now.

See also subtypes like sequencePart, I'm not sure how this influences the usage / import of data.

juliuste commented 7 years ago

Doesn't include #19, #4 or #16 yet.

juliuste commented 7 years ago

Closes #3

kiliankoe commented 7 years ago

Just leaving this here for reference: ProtoBuf Oneof

Nested types like SequencePart will be necessary to model that correctly, possibly SequenceElement or just Element as it will be listed in the field sequence anyways?

I'd really like to try out the Oneof nesting, since station can reference region which can reference station, which could lead to endless nesting - obviously doesn't make sense, but it's possible to express.

Otherwise: Yay for protobuf definitions ๐ŸŽ‰

derhuerst commented 7 years ago

Will submit tests based in the .proto file in another PR soon.

juliuste commented 7 years ago

Updated the proto file.

derhuerst commented 7 years ago

So I just played around with it. The oneof feature cannot be used. It does not support a mechanism like function overloading (one property with two different possible types). We're doomed. ๐Ÿ˜›

juliuste commented 7 years ago

Cap'n Proto seems to allow true union types, we could use it insteadโ€ฆ

kiliankoe commented 7 years ago

Just tried the following.

message Station {
    string id = 1;
    string name = 2;
    Coordinates coordinates = 3;
    string address = 4;
    oneof regions {
        string reference = 5;
        Region region = 6;
    }
}

I had to remove all optional and required declarations for protobuf 3, no clue what that was about (again, I don't have much experience with protobuf ๐Ÿ™ˆ), but the oneof seemed to work fine. Or is the declaration of that wrong?

kiliankoe commented 7 years ago

Oh wait, repeated won't work there ๐Ÿ˜•

derhuerst commented 7 years ago

My point is that you can't use oneof to assign two possible types to one name such as region. You have to use region & regionRef.

repeated doesn't work inside a oneof declaration.

derhuerst commented 7 years ago

I see two reasons to add protocol buffer markup:

It seems to me that for now, the first reason is more important. We try create a format that is usable across languages in order to be truly useful. And by not having to write type declarations for statically typed languages, we lower the threshold for others to use FPTF.

@juliuste Using Cap'n Proto would be fine, I don't see any reason not to do this. As the encoding is less wide-spread however, I think we would have to add type declarations for languages in addition.


Moving on, I see four ways to achieve the goals mentioned above:

  1. Use two separate fields station and stationRef and declare that only one of them is allowed. We would be able to use protocol buffers and any language that does not have union types.
  2. Use Cap'n Proto and optionally add language-specific type declarations.
  3. Ditch protocol buffers and Cap'n Proto altogether. E.g. validate-fptf works without type definitions. Optionally add language-specific type declarations.
kiliankoe commented 7 years ago

Whilst option 1 (as in protobuf support) would definitely be nice to have, I do find the inline values rather elegant. Obviously some languages will make it harder to implement, but even those languages that don't offer any kind of union types will just need a bit of extra typecasting to make working with this format possible.

Otherwise my vote would be for option 3 and relying on community support to write and maintain libs for different languages.

derhuerst commented 7 years ago

I'm with option 2 or 3 as well. I don't mind if someone added Cap'n Proto definitions, but I wouldn't consider them the source of truth either.

derhuerst commented 7 years ago

Will close this for now, as the .proto file is not really usable (yet?).