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

Editorial fix: standardize reporting of conditional requirements in GTFS Schedule #271

Merged
merged 2 commits into from May 20, 2021

Conversation

scmcca
Copy link
Contributor

@scmcca scmcca commented May 6, 2021

This PR standardizes the reporting of "Conditionally Required:" statements in field descriptions and adds the convention where it was previously missing (i.e., stop_times.arrival_time, .departure_time). The aim is to increase the legibility of the documentation.

Contained in this PR:

  • Standardized reporting of conditional requirements in the body of field descriptions.
  • Standardized list styles.
  • Replaced "Required" heading with "Presence".
  • Structured "Document Conventions" section.

Feedback welcomed!

- Standardized reporting of conditional requirements in the body of field descriptions.
- Standardized list styles.
- Replaced "Required" heading with "Presence".
- Structured "Documentation Conventions" section.
@google-cla google-cla bot added the cla: yes label May 6, 2021
@scmcca
Copy link
Contributor Author

scmcca commented May 11, 2021

Follow-up: While no changes are being made to the content of the spec, we (MobilityData) invite a general review of the changes, and whether the community thinks a vote is necessary in this case. If not, we will move forward with merging the proposed fixes in about a week. Thanks!

@skinkie
Copy link
Contributor

skinkie commented May 11, 2021

What I dislike from this patch is that it mixes aesthetic (letter cases) with content changes, this must be avoided.

@brodyFlannigan
Copy link

+1 Brody Flannigan Transit Data.

@gcamp
Copy link
Contributor

gcamp commented May 12, 2021

Agree with @skinkie + if possible in the future the 4 changes listed could have been 4 PRs to make the review substantially easier. Alternatively, it could be 4 different commits that could be reviewed individually.

That said, I'm ok with the changes. The changes don't require a vote in my opinion.

@scmcca
Copy link
Contributor Author

scmcca commented May 12, 2021

@skinkie The challenge is standardizing the reporting of "Conditionally Required" statements in field descriptions where they were previously embedded, this inherently changes the form of the content, but should not change the meaning or interpretation of the spec. Beyond that, I understand that restructuring a "Document Conventions" heading as well as replacing "Required" headings with "Presence" is more than an aesthetic fix. However, the end goal is to make the documentation more clear, so if there are specific areas that were made less clear in this process, please let me know and we can adjust them!

- fixed markdown heading typo
- removed double spacing after conditionally required in route_long_name
@scmcca
Copy link
Contributor Author

scmcca commented May 18, 2021

@gcamp Great point! Separating these changes out would have been better.

If the changes look good and the consensus is that a vote is not needed, we'll merge them later this week. Feel free to provide any remaining feedback. Thanks!

@timMillet timMillet merged commit a968ee5 into google:master May 20, 2021
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.

None yet

5 participants