Skip to content

feat: recommended file annotation #877#1123

Closed
KClough wants to merge 1 commit intoMobilityData:masterfrom
JarvusInnovations:feat/recommended-file-annotation
Closed

feat: recommended file annotation #877#1123
KClough wants to merge 1 commit intoMobilityData:masterfrom
JarvusInnovations:feat/recommended-file-annotation

Conversation

@KClough
Copy link
Copy Markdown
Contributor

@KClough KClough commented Apr 25, 2022

Summary:

Resolves #877

Adds a new annotation for recommended files and applies this annotation to feed_info.txt and shapes.txt

Expected behavior:

A new validator warning is generated when either feed_info.txt or shapes.txt is missing

Please make sure these boxes are checked before submitting your pull request - thanks!

In liue of a screenshot, here is the sample output from the validator for a data set with missing feed_info.txt and shapes.txt:

{
  "notices": [
    {
      "code": "missing_recommended_file",
      "severity": "WARNING",
      "totalNotices": 2,
      "sampleNotices": [
        {
          "filename": "shapes.txt"
        },
        {
          "filename": "feed_info.txt"
        }
      ]
    }
]}

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 25, 2022

CLA assistant check
All committers have signed the CLA.

@KClough KClough force-pushed the feat/recommended-file-annotation branch 2 times, most recently from 5e59b46 to b84e7e2 Compare April 25, 2022 19:56
Copy link
Copy Markdown
Contributor

@isabelle-dr isabelle-dr left a comment

Choose a reason for hiding this comment

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

Thanks you for this great contribution @KClough!
I had a quick look because I know you are planning on adding more rules (🎉!) so I wanted to see if there are corrections that would apply to your next contributions.

Besides the comments that I left in-line, I have two main pieces of feedback here:

  1. There are two things included in this PR: the addition of the @recommended annotation in this validator, and the addition of a new validator notice. If this isn't too much effort on your end, I would recommend splitting this PR in two to make sure we aren't missing anything during the reviews.
  2. As mentioned in the issue this PR is closing (#887), I believe there is no mention in (1) the spec or (2) the Best Practices that shapes.txt should be included. This validator is currently based on them, and the few exceptions we have are rules that are related to good data design practices (e.g. checking that pathway is not a loop). The recommendation on shapes.txt should be added in the Best Practices first before being added here, so I recommend removing it from this PR.

Our tech lead will have a look shortly to look at the technical aspects.
Thank you so much again to contribute to this tool, let me know if you have any questions!

* <p>Example.
*
* <pre>
* {@literal @}GtfsTable("agency.txt")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would have a real example here with a file where we use the @recommended annotation. It could be feed_info.txt.

RULES.md Outdated
| [`LocationWithUnexpectedStopTimeNotice`](#LocationWithUnexpectedStopTimeNotice) | A location in `stops.txt` that is not a stop is referenced by some `stop_times.stop_id`. |
| [`MissingCalendarAndCalendarDateFilesNotice`](#MissingCalendarAndCalendarDateFilesNotice) | Missing GTFS files `calendar.txt` and `calendar_dates.txt`. |
| [`MissingLevelIdNotice`](#MissingLevelIdNotice) | `stops.level_id` is conditionally required. |
| [`MissingRecommendedFileNotice`](#MissingRecommendedFileNotice) | A recommended file is missing. |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are missing the part this link brings the user to. It is usually a longer description and it appears lower in the document. It could be:

MissingRequiredFileNotice

The GTFS Best Practice recommends a file be included, but it's missing.

References:

GTFS Best Practices

Copy link
Copy Markdown
Contributor

@isabelle-dr isabelle-dr May 4, 2022

Choose a reason for hiding this comment

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

⚠️ @KClough We are modifying the structure of RULES.md in PR #1132. I suggest we wait until it is merged before you apply the changes to RULES.md.

docs/NOTICES.md Outdated
| `location_without_parent_station` | [`LocationWithoutParentStationNotice`](#LocationWithoutParentStationNotice) |
| `missing_calendar_and_calendar_date_files` | [`MissingCalendarAndCalendarDateFilesNotice`](#MissingCalendarAndCalendarDateFilesNotice) |
| `missing_level_id` | [`MissingLevelIdNotice`](#MissingLevelIdNotice) |
| `missing_recommended_file` | [`MissingRecommendedFileNotice`](#MissingRecommendedFileNotice)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar to my comment in RULES.md, we are missing the part at the bottom of the document, where we give the user a table showing what is included in the JSON schema for this notice.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ @KClough We are deprecating NOTICES.md in PR #1132

@KClough KClough force-pushed the feat/recommended-file-annotation branch 4 times, most recently from 15cb119 to b8116f6 Compare May 6, 2022 21:23
@KClough KClough force-pushed the feat/recommended-file-annotation branch from b8116f6 to 5d191ee Compare May 16, 2022 22:42
@KClough KClough closed this May 16, 2022
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.

Best Practice: feed_info.txt should be provided (WARNING)

4 participants