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

feat: Realtime schema updated in README #150

Merged
merged 8 commits into from
May 25, 2022
Merged

Conversation

emmambd
Copy link
Contributor

@emmambd emmambd commented May 10, 2022

Based on the discussion in issue #36, this is the updated schema that will be shared in the README.

@emmambd emmambd requested a review from maximearmstrong May 10, 2022 20:17
@emmambd emmambd linked an issue May 10, 2022 that may be closed by this pull request
7 tasks
@emmambd emmambd mentioned this pull request May 10, 2022
Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

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

Some comments in-line before approval.

I strongly suggest that we add a Type column right after the Field column. We can use the types described at gtfs.org.

We will also need the JSON type in some cases. For example, the type for a realtime URL would be JSON, and it would follow its own data structure, like GTFS Realtime URL data structure, which can be referenced in the Description column for the row. You could define this in another table in a subsection with direct_download_url, license_url, etc. The [Transitfeeds] API (https://transitfeeds.com/api/swagger/) follows this idea and works well.

I think it would make the data structure clearer and also avoid duplicates.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@barbeau barbeau left a comment

Choose a reason for hiding this comment

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

Thanks @emmambd, some comments in-line.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

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

A few things to consider before modifying the JSON schema :) Let's discuss them.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

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

One small change to make :) Aside that, LGTM!

README.md Outdated Show resolved Hide resolved
maximearmstrong and others added 2 commits May 25, 2022 13:25
Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>
@emmambd
Copy link
Contributor Author

emmambd commented May 25, 2022

@maximearmstrong Thanks for the update - should be set now!

@maximearmstrong maximearmstrong merged commit 4c452bc into main May 25, 2022
@maximearmstrong maximearmstrong deleted the realtime-schema branch May 25, 2022 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define realtime schema and operations
3 participants