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

Best practices: add recommended presence #386

Merged
merged 13 commits into from
Aug 1, 2023

Conversation

emmambd
Copy link
Collaborator

@emmambd emmambd commented Jul 5, 2023

Problem and Suggested Scope

Referenced from #375 and #376:

MobilityData’s heard a number of pains from the community about the GTFS best practices living in two different places: the spec's SHOULD statements, and the GTFS Best Practices.

  • Stakeholders are not always aware of the existence of the best practices, and so moving these into the official spec would give the best practices greater visibility and improve data quality for everyone

  • Merging the best practices in the spec would make it easier for regulators to point producers to one place to get the information they need to create their GTFS feeds

We’ve been exploring what a good starting “release” or iteration would look like to begin this work. Based on feedback in #375 and some other discussions, it looks like the one key starting point is providing clear guidelines in the specification for when a field or file is recommended for use.

Before creating this PR, we did an initial review of all places in the spec and best practices where they offer advice on when to add a file or field. This spreadsheet also includes the corresponding Canonical Validator rule for each best practice for cross-referencing.

Proposed solution

Adding a Recommended presence in the spec that conforms to RFC conventions. This would make it possible to clearly state that a field or file is not required, but adding it is a best practice that should be considered. Making it immediately obvious that a file or field is recommended in the spec reduces confusion, while preserving backwards compatibility.

This change would not affect validity or the quality evaluation of the dataset according to the Canonical Validator, since the best practices are already translated to warnings in the validator. Any missing recommended field or file would generate a warning in the validator.

If the associated PR is adopted, MobilityData will open a PR on https://github.com/MobilityData/GTFS_Schedule_Best-Practices so that all best practices that have been reconciled + adopted into the spec are removed from the best practices repo.

When is a file/field Recommended instead of Optional?

The Recommended presence is used for any file/field that should be used universally, e.g feed_info.txt or agency_id. This is information producers should provide in all circumstances, but is not required in order to preserve backwards compatibility.

Any file/field that is recommended only under specific circumstances (such as route_short_name when there is a brief service designation) keeps the Optional presence and the words "SHOULD" and/or "recommended" are used in its description.

Alternatives considered

Adding no additional presences and keeping all SHOULDs in the description
The concern with this approach was that it remains confusing for producers for a file/field to be optional but recommended in all cases. Conversation here.

Adding a Conditionally Recommended presence
This was originally drafted, this seemed to overly complicate the spec and create confusion about the difference between Recommended presences and the Optional presence. Conversation here.

@emmambd emmambd added the GTFS Schedule Issues and Pull Requests that focus on GTFS Schedule label Jul 5, 2023
Copy link
Contributor

@gcamp gcamp left a comment

Choose a reason for hiding this comment

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

👏 left a couple comments

gtfs/spec/en/reference.md Outdated Show resolved Hide resolved
gtfs/spec/en/reference.md Outdated Show resolved Hide resolved
gtfs/spec/en/reference.md Outdated Show resolved Hide resolved
@Sergiodero
Copy link
Collaborator

Sergiodero commented Jul 20, 2023

Hey @gcamp, Sergio from MobilityData here. We've added a new commit that removes the May be provided for frequency-based trips text. We’re proposing to remove this and instead incorporate a use case in the GTFS.org example section to illustrate the use of block_id with frequency based trips, this example could be similar to the one @antrim shared in issue #227.

Currently we’re exploring what could be a good example or use case, but we think that this could be a separate discussion as it would fall on the GTFS.org repo, allowing us to move forward with the discussion in this PR, while simultaneously working on defining this example.

@emmambd
Copy link
Collaborator Author

emmambd commented Jul 24, 2023

Since all PR comments have been resolved and there's been engagement and interest in #376 and #376, I'm opening the vote. This PR adds a recommended presence to the spec and best practices for when specific files and fields should be added. These guidelines are taken from GTFS Best Practices.

Please vote with a +1 (for) or -1 (against) in the comments. Voting ends on 2023-07-31 at 23:59:59 UTC.

@emmambd emmambd added the Status: Voting Pull Requests where the advocate has called for a vote as described in the changes.md label Jul 24, 2023
@gcamp
Copy link
Contributor

gcamp commented Jul 24, 2023

+1 Transit

@markstos
Copy link
Contributor

+1 RideAmigos

@westontrillium
Copy link
Contributor

+1 Trillium

@derhuerst
Copy link

+1 bbnavi

@doconnoronca
Copy link

+1 TransSee

@NomeQ
Copy link

NomeQ commented Jul 27, 2023

+1 Garnet

@isabelle-dr isabelle-dr mentioned this pull request Jul 27, 2023
4 tasks
@evansiroky
Copy link
Contributor

+1 Cal-ITP.

@philip-cline
Copy link

+1 Arcadis IBI

@bdferris-v2
Copy link
Collaborator

+1 Google

One formatting nit: for the shapes.txt - shape_dist_traveled addition, was the intention that the "Example" be rendered inside the table cell for the shape_dist_traveled field? If so, I think you need to get rid of the line break before the <hr>*Example line because it causes the content to break out of the table cell.

Screenshot 2023-07-31 at 3 10 39 PM

@emmambd
Copy link
Collaborator Author

emmambd commented Jul 31, 2023

@bdferris-v2 Thanks for catching that! Corrected.

@emmambd
Copy link
Collaborator Author

emmambd commented Aug 1, 2023

The vote passed on 2023-07-31 at 23:59:59 UTC.

9 votes in favour and no votes against.

The votes came from:
Transit (@gcamp)
RideAmigos (@markstos)
Trillium (@westontrillium)
bbnavi (@derhuerst)
TransSee (@doconnoronca)
Garnet (@NomeQ)
Cal-ITP (@evansiroky)
Arcadis IBI (@philip-cline)
Google (@bdferris-v2)

Thanks to everyone who contributed and voted! There will be a PR on the Best Practices repo to remove these from the best practices now that they're duplicated. You can follow the progress on this here.

@emmambd emmambd merged commit 17a0321 into google:master Aug 1, 2023
2 checks passed
@emmambd emmambd removed the Status: Voting Pull Requests where the advocate has called for a vote as described in the changes.md label Aug 1, 2023
@emmambd
Copy link
Collaborator Author

emmambd commented Aug 23, 2023

👋 Here's an update to go vote to remove these same best practices from the Best Practices repo so we don't have duplicate guidance: MobilityData/GTFS_Schedule_Best-Practices#59.

Voting ends on 2023-08-29 at 23:59:59 UTC.

gcamp pushed a commit to TransitApp/transit that referenced this pull request Sep 25, 2023
* Add recommended presence

* feed_info.txt best practices

* route_short_name best practice

* agency_id best practice

* timepoint best practice

* inlining image for shape_dist_traveled

* shape_dist_traveled field best practices

* block_id field presence best practice

* Clearer recommended definition

* clarity improvements

* Remove "may" for block_id in frequency based trips

* fix shape_dist_traveled cell formatting
@isabelle-dr isabelle-dr linked an issue Apr 25, 2024 that may be closed by this pull request
@isabelle-dr isabelle-dr added the Change: Best Practice Changes focusing on recommendations for optimal use of the specification. label Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Change: Best Practice Changes focusing on recommendations for optimal use of the specification. GTFS Schedule Issues and Pull Requests that focus on GTFS Schedule
Projects
None yet