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

Move stats in a metadata.stats objects #192

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Conversation

antoine-de
Copy link
Member

And add more counts:

  • trips_with_trip_headsign_count: usize,
  • lines_with_short_name_count: usize,
  • lines_with_long_name_count: usize,
  • transfers_count: usize,
  • fares_attribute_count: usize,

There is no fare_rules yet, they are not handled by gtfs structures yet

fixes #188 #189 #190 and partially #191 since we need to handle fare_rules

And add more counts:
* trips_with_trip_headsign_count: usize,
* lines_with_short_name_count: usize,
* lines_with_long_name_count: usize,
* transfers_count: usize,
* fares_attribute_count: usize,

There is no `fare_rules` yet, they are not handled by gtfs structures
yet
@antoine-de antoine-de requested review from Tristramg and a team as code owners January 26, 2024 21:45
Copy link
Member

@AntoineAugusti AntoineAugusti left a comment

Choose a reason for hiding this comment

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

Thank you for your work Antoine! A few things

src/metadatas.rs Show resolved Hide resolved
src/metadatas.rs Show resolved Hide resolved
src/metadatas.rs Show resolved Hide resolved
@antoine-de
Copy link
Member Author

I added the fare rules count too as it has been merged in gtfs-structures (in rust-transit/gtfs-structure#157)

By curiosity, do you know if there are producers that have implemented the fares v2 model?

@AntoineAugusti
Copy link
Member

AntoineAugusti commented Feb 12, 2024

Thanks for the changes!

By curiosity, do you know if there are producers that have implemented the fares v2 model?

Not yet 😢 Grand Est / Fluo may be the first in the coming months, they are working with Cityway on it.

Copy link
Member

@AntoineAugusti AntoineAugusti left a comment

Choose a reason for hiding this comment

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

stats.stops_with_wheelchair_info_count needs to be set, currently it's hardcoded to null

src/metadatas.rs Outdated Show resolved Hide resolved

pub stats: Stats,
// legacy fields, now counter should be in stats
pub stop_areas_count: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

are those legacy fields used elsewhere, shouldn’t they be removed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's to maintain API retro compatibility, I don't think we can remove those

Copy link
Member Author

Choose a reason for hiding this comment

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

@AntoineAugusti do you think we could remove those legacy fields after the integration in the transport site is done ?
I'm always wary of API breaking, but @Tristramg might have a point that this should not impact anybody other than https://github.com/etalab/transport-site

Copy link
Member

Choose a reason for hiding this comment

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

Yup, the legacy *_count fields outside of the new stats key can be removed in the next PR!

@AntoineAugusti
Copy link
Member

@antoine-de Can you push the latest commit on the stats branch? To make sure things are fixed 👌

@antoine-de
Copy link
Member Author

done, sorry forgot this

Copy link
Member

@AntoineAugusti AntoineAugusti left a comment

Choose a reason for hiding this comment

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

The validator output looks fine!

@antoine-de
Copy link
Member Author

nice, can I merge?

@AntoineAugusti
Copy link
Member

Yup, looks good.

Should we bump the version? Or in the next PR when removing attributes?

@antoine-de
Copy link
Member Author

since it's a binary and not a library, we have never kept clean version in this, but you're right, maybe we can update the version after removing the fields

@antoine-de antoine-de merged commit 61912cf into etalab:master Feb 20, 2024
1 check passed
@antoine-de antoine-de mentioned this pull request Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants