Conversation
| @@ -2,13 +2,13 @@ | |||
|
|
|||
| **Revised August 22, 2018. See [Revision History](../../CHANGES.md) for more details.** | |||
There was a problem hiding this comment.
Future process reminder for us: We need to remember to update the revision history when pull requests are ultimately approved.
gtfs/spec/en/reference.md
Outdated
| * **Field conditionally required** - This field or file is **required** under certain conditions, which are outlined in the field or file *Details*. Outside of these conditions, this field or file is optional. | ||
| * **Dataset unique** - The field contains a value that maps to a single distinct entity within the column. For example, if a route is assigned the ID **1A**, then no other route may use that route ID. However, you may assign the ID **1A** to a location because locations are a different type of entity than routes. | ||
| * **Feed** - Successive datasets comprise a feed. | ||
| * **Dataset** - A complete set of files defined by this specification reference. Datasets should be published at a public, permanent URL, including the zip file name. (e.g., www.agency.org/gtfs/gtfs.zip). For more information see GTFS best practices. |
There was a problem hiding this comment.
Put the "Dataset" definition above "Feed" since the "Feed" definition refers to it.
There was a problem hiding this comment.
I agree with the definition, but isn't this a change to the existing spec? We should strive to avoid addition to the spec in this PR IMHO.
There was a problem hiding this comment.
Agreed, I'm not sure the "Feed" definition will be universally agreed-upon. We should probably vote on that separately.
There was a problem hiding this comment.
I'd expand the example to the full URL - https://www.agency.org/gtfs/gtfs.zip
There was a problem hiding this comment.
I could see how this would be debated, but I also am having a hard time seeing alternative definitions that are usable. The definitions we have proposed are based on existing usage, albeit quite inconsistent usage.
schema.org's uses of the terms "data feed" and "dataset" seem to match what we are doing:
Basically, I think that we should be bold in this case. The terms "dataset" and "feed" have been consistently applied in this proposed spec modification. We could revert, but it hurts to leave this inconsistency in the Spec.
An idea: Prior to the pull request shall we bring the question to a somewhat broader group to see if they agree with our change?
gtfs/spec/en/reference.md
Outdated
| * **Field** - A specific type of information for records in a .txt file. Represented, in a table, as a column. | ||
| * **Record** - A basic data structure comprised of a number of different fields describing a single entity (e.g. transit agency, stop, route, etc.). Represented, in a table, as a row. | ||
| * **Field Value** - An individual entry in a field. Represented, in a table, as a single cell. | ||
| * **Required** - The field must be included in the dataset, and a value must be provided for each field. Some required fields permit an empty string as a value (denoted in this specification as "empty"). To enter an empty string, just omit any text between the commas for that field. Note that `0` is interpreted as "a string of value 0", and is not an empty string. See the field definition for details. |
There was a problem hiding this comment.
I think this is unnecessary:
"Note that 0 is interpreted as "a string of value 0", and is not an empty string. See the field definition for details."
There was a problem hiding this comment.
"for each field" ? You mean for each "field value", no?
Some required fields permit an empty string as a value
I don't think so. I would call them optional. But maybe I'm missing something
There was a problem hiding this comment.
"for each field" ? You mean for each "field value", no?
It should be "field value", good catch.
Some required fields permit an empty string as a value
I don't think so. I would call them optional. But maybe I'm missing something
transfers in fare_attributes.txt and transfer_type in transfers.txt are both listed as required and permit empty field values as an enum option.
There was a problem hiding this comment.
IMHO and a value must be provided for each field should be and a value must be provided in that field for each record.
There was a problem hiding this comment.
Good catch. We could test fixing that but through a different pull request / discussion.
antrim
left a comment
There was a problem hiding this comment.
@giocorti : I have finished my review. Looks good! I have requested minor changes, and opened a few questions for discussion.
@barbeau @LeoFrachet Do you have any comments? Or should we discuss a few of the questions I raised in an internal meeting?
gtfs/spec/en/reference.md
Outdated
| * **Field** - A specific type of information for records in a .txt file. Represented, in a table, as a column. | ||
| * **Record** - A basic data structure comprised of a number of different fields describing a single entity (e.g. transit agency, stop, route, etc.). Represented, in a table, as a row. | ||
| * **Field Value** - An individual entry in a field. Represented, in a table, as a single cell. | ||
| * **Required** - The field must be included in the dataset, and a value must be provided for each field. Some required fields permit an empty string as a value (denoted in this specification as "empty"). To enter an empty string, just omit any text between the commas for that field. Note that `0` is interpreted as "a string of value 0", and is not an empty string. See the field definition for details. |
There was a problem hiding this comment.
"for each field" ? You mean for each "field value", no?
Some required fields permit an empty string as a value
I don't think so. I would call them optional. But maybe I'm missing something
|
@giocorti Amazing job, thanks! I only reviewed until end of They are many small things to tighten up. But we're going somewhere. |
IIRC GitHub's Markdown renderer shows columns with equal width no matter the content, so 2 columns ends up wasting a lot of space when viewing on GitHub. Also, generally, I think 4 columns is easier to digest for newcomers to the spec, so I'd suggest keeping that. Idea - in the HTML-rendered site, we could offer a "collapsed" view for pros that shows only two columns in the format the Leo is using. I think that would be relatively straightforward (?) if we encode the info in 4 columns in Markdown. |
|
I've reviewed all the suggestions and made the changes summarized below. Thanks for all the input! LMK what you think.
|
|
I'm rereading it completely before pushing it to the public repo. |
|
LGTM, with the three small comments above. |
|
LGTM ! |




This PR makes the following editorial and formatting changes to the GTFS specification.
parent_stationstop_timezonearrival_timeanddeparture_timeMonday,...,Sundaytimepointexact_timesAs you can see, I made a lot of changes and I'm sure we'll need to discuss some of them. Let me know what you think.