Add v2.3 support to Rust

[ryanzomorrodi]

Hi all. I needed v2.3 rust support for a project I was working on, so I used the existing rust crate as a framework to build off. I'm not sure if you all would like to add support for earlier versions, but I thought I'd open up a pull request just in case my work would be helpful to others. No worries if not

[CLAassistant]

All committers have signed the CLA.

[richfab]

Hi @ryanzomorrodi,

I think it's a great idea to add support for v2.3. Thank you for sharing your work!

@tdelmas are you up for a code review? :)

cc @emmambd

Fabien (MobilityData)

[tdelmas]

The rust part looks good to me. (I don't have time to check the conformance with the specification, but it looked ok at a first glance)

[richfab]

Hello @ryanzomorrodi,

I've asked our Tech Lead, @davidgamez, to review your PR.

Thanks again for sharing your work with the community.

Best regards,
Fabien (MobilityData)

[Alessandro100]

The rust aspect looks good, my comments focus purely on the spec

[ryanzomorrodi]

Hi @Alessandro100 I left these as optional mostly for consistency with the other versions.

For example,

In the v3 system_information.json all of these variables are required

Field Name	REQUIRED	Type	Defines
languages (added of v3.0)	Yes	Array<Language>	List of languages used in translated strings.
name (as of v3.0)	Yes	Array<Localized String>	Name of the system to be displayed to customers.
opening_hours (added in v3.0)	Yes	String	Hours and dates of operation for the system in OSM opening_hours format. (added in v3.0)

But in the v3 rust model all of these are optional

    pub languages: Option<Vec<Language>>,
    /// Name of the system to be displayed to customers.
    pub name: Option<Vec<LocalizedString>>,
    /// Hours and dates of operation for the system in [OSM opening_hours](https://wiki.openstreetmap.org/wiki/Key:opening_hours) format.
    pub opening_hours: Option<OSMOpeningHours>,

If you'd like though, I can make all of these required.

[tdelmas]

I would argue that for old version, it is probably better to be more flexible as there is more non-compliant old feeds, that would be unreadable if the types are too strict.

[Alessandro100]

The language bindings should be a reflection of the spec, I don’t think we should build with the idea that certain feeds would not be spec compliant, what are your thoughts @richfab

On @tdelmas 's point, I'd treat feed tolerance as a parsing concern, not a struct concern. If required fields are Option (or default silently), the binding can no longer tell us whether a feed was actually compliant, and every consumer has to defensively check fields the spec guarantees

@ryanzomorrodi Thank you for bringing up the similar issues coming from v3.0, a ticket was created to correct this (#204). I’ll hold off asking for changes until we land on a decision

[richfab]

Although I understand the point of you of @tdelmas for consuming older feeds, I agree that the language bindings should be a reflection of the spec for all versions.

[Alessandro100]

@ryanzomorrodi go ahead and make those changes that will conform it to the spec

[testower]

Just to chime in - I made the Java model more permissive than the spec for precisely the reason @tdelmas outlined. It does not enforce required fields.

[richfab]

Thank you @testower for your input. I understand the need to be able to consume feeds with missing required fields.

However, I believe that reflecting the spec is a good way to ensure required fields are present when the language bindings are used to produce the data.

Question for a separate issue: What technical solutions could satisfy both data producers and consumers?

[testower]

Thank you @testower for your input. I understand the need to be able to consume feeds with missing required fields.

However, I believe that reflecting the spec is a good way to ensure required fields are present when the language bindings are used to produce the data.

Question for a separate issue: What technical solutions could satisfy both data producers and consumers?

I don't think there is a clear answer to that question. The java model currently does not enforce required fields. And I don't think there is a way to only enforce it on serialization other than having separate models. jsonschema2pojo doesn't seem to support this distinction.

[richfab]

After internal consultation, we will keep the language bindings behaviour consistent across languages and make it more permissive for Rust (do not enforce required fields).

@Alessandro100 Can you please resolve the related comments?

Thanks!

cc @emmambd

[davidgamez]

I'm in favor of having permissive bindings. The role of the bindings is to help consumers, not enforce validation. Validation should be enforced by other tools, aka validators.

[ryanzomorrodi]

Sounds like you all are now in favor of permissive bindings. I think @Alessandro100 pointed out two fields that should be made optional according to the spec so let me fix those before merging

[ryanzomorrodi]

Since you all are already having this discussion. I have noticed some producers will use integers instead of booleans. Is this something you all would want to cover?

Example: https://gbfs.lyft.com/gbfs/2.3/chi/en/station_status.json

[richfab]

@ryanzomorrodi For consistency, I'd recommend following the same pattern as other the other Rust versions of the language biddings. Thank you

[Alessandro100]

@ryanzomorrodi bump the cargo.toml version to 0.1.5 and I'm good for the approval

[Alessandro100]

Looks good, and thank you for your contribution ✅