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: 1640 add remaining gtfs features to the validator #1656

Merged
merged 15 commits into from
Jan 24, 2024

Conversation

qcdyx
Copy link
Contributor

@qcdyx qcdyx commented Jan 22, 2024

Summary:

Added new GTFS features as described in #1640

Expected behavior:
MBTA.zip
image
Bullrunner.zip
image

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

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Add or update any needed documentation to the repo
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@qcdyx qcdyx linked an issue Jan 22, 2024 that may be closed by this pull request
8 tasks
Copy link
Contributor

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

@qcdyx qcdyx requested review from emmambd and jcpitre January 22, 2024 16:06
@qcdyx qcdyx assigned qcdyx and jcpitre and unassigned qcdyx Jan 22, 2024
Copy link
Contributor

@emmambd emmambd left a comment

Choose a reason for hiding this comment

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

@qcdyx Nice work! Two comments:

  • I don't see Pathways (extra) when I test with MBTA's feed
  • I now get a agency.txt. INVALID_HEADERS error when I run the validator through the cli. This is making the agency column in the Summary report empty even when the agency is provided by the feed (which I can also see in your screenshot). I don't see this in our staging environment, so I'm wondering if this is introducing a new bug.

@emmambd
Copy link
Contributor

emmambd commented Jan 22, 2024

@qcdyx A requirement change: Transfer Rules should be renamed "Transfer Fares"

Copy link
Contributor

❌ Invalid acceptance test.
New Errors: 1418 out of 1476 datasets (~96%) are invalid due to code change, which is above the provided threshold of 1%.
Dropped Errors: 21 out of 1476 datasets (~1%) are invalid due to code change, which is above the provided threshold of 1%.
New Warnings: 6 out of 1476 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 71 out of 1476 datasets (~5%) are invalid due to code change, which is above the provided threshold of 1%.
3 out of 1479 sources (~0 %) are corrupted.
Corrupted sources:
ae-abu-dhabi-emirate-department-of-municipalities-and-transport-gtfs-1329
gb-inverclyde-mcgills-buses-gtfs-1941
ie-donegal-dohertys-coach-travel-gtfs-973
Commit: cdd5962
Download the full acceptance test report here (report will disappear after 90 days).
❌ Invalid acceptance test.

@qcdyx
Copy link
Contributor Author

qcdyx commented Jan 22, 2024

image

Copy link
Contributor

❌ Invalid acceptance test.
New Errors: 1422 out of 1476 datasets (~96%) are invalid due to code change, which is above the provided threshold of 1%.
Dropped Errors: 19 out of 1476 datasets (~1%) are invalid due to code change, which is above the provided threshold of 1%.
New Warnings: 6 out of 1476 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 67 out of 1476 datasets (~5%) are invalid due to code change, which is above the provided threshold of 1%.
3 out of 1479 sources (~0 %) are corrupted.
Corrupted sources:
br-minas-gerais-empresa-de-transportes-e-transito-de-belo-horizonte-bhtrans-gtfs-9
us-hawaii-thebus-gtfs-10
us-vermont-lake-champlain-ferries-gtfs-547
Commit: 314aebb
Download the full acceptance test report here (report will disappear after 90 days).
❌ Invalid acceptance test.

@davidgamez
Copy link
Member

❌ Invalid acceptance test. New Errors: 1422 out of 1476 datasets (~96%) are invalid due to code change, which is above the provided threshold of 1%. Dropped Errors: 19 out of 1476 datasets (~1%) are invalid due to code change, which is above the provided threshold of 1%. New Warnings: 6 out of 1476 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%. Dropped Warnings: 67 out of 1476 datasets (~5%) are invalid due to code change, which is above the provided threshold of 1%. 3 out of 1479 sources (~0 %) are corrupted. Corrupted sources: br-minas-gerais-empresa-de-transportes-e-transito-de-belo-horizonte-bhtrans-gtfs-9 us-hawaii-thebus-gtfs-10 us-vermont-lake-champlain-ferries-gtfs-547 Commit: 314aebb Download the full acceptance test report here (report will disappear after 90 days). ❌ Invalid acceptance test.

I expected no changes in the acceptance tests as #1640 tasks only relate to adding component information to the HTML and JSON formatting. Is it related to

public interface GtfsNetworkSchema extends GtfsEntity {
?

@emmambd
Copy link
Contributor

emmambd commented Jan 22, 2024

@qcdyx Additional changes to scope (could make a new issue/PR for this one):

Remove the following base components:

  • Route Names
  • Agency Information

@qcdyx
Copy link
Contributor Author

qcdyx commented Jan 23, 2024

@emma just fixed the agency problem
image
image

@emmambd
Copy link
Contributor

emmambd commented Jan 23, 2024

@qcdyx Rename Pathways to Pathways (basic)

changed loadPathwayExtraComponent logic
Copy link
Contributor

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

@qcdyx
Copy link
Contributor Author

qcdyx commented Jan 23, 2024

renamed Pathways to Pathways (basic)
image

Copy link
Contributor

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

@qcdyx qcdyx requested a review from davidgamez January 23, 2024 19:29
@qcdyx
Copy link
Contributor Author

qcdyx commented Jan 23, 2024

@davidgamez I've made the changes and commented GtfsNetworkSchema code. I'll uncommented the code and remove the requested components in a new PR once this is merged.

@jcpitre jcpitre assigned qcdyx and unassigned jcpitre Jan 23, 2024
@davidgamez
Copy link
Member

@davidgamez I've made the changes and commented GtfsNetworkSchema code. I'll uncommented the code and remove the requested components in a new PR once this is merged.

Thanks @qcdyx , Can you kindly remove the commented code from the PR?

@qcdyx qcdyx dismissed davidgamez’s stale review January 23, 2024 20:01

Changes commited

Copy link
Contributor

✅ Rule acceptance tests passed.
New Errors: 0 out of 1481 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Errors: 0 out of 1481 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
New Warnings: 0 out of 1481 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
Dropped Warnings: 0 out of 1481 datasets (~0%) are invalid due to code change, which is less than the provided threshold of 1%.
0 out of 1481 sources (~0 %) are corrupted.
Commit: 49b0b57
Download the full acceptance test report here (report will disappear after 90 days).
✅ Rule acceptance tests passed.

Copy link
Member

@davidgamez davidgamez left a comment

Choose a reason for hiding this comment

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

This PR looks good to me. I'll defer to @emmambd and @jcpitre for more deep testing regarding the feature logic.

@emmambd
Copy link
Contributor

emmambd commented Jan 24, 2024

@qcdyx @davidgamez The example feeds all run correctly - only thing I notice is that Route Names hasn't been deleted. I'll let @jcpitre do the final review + merge :)

@qcdyx
Copy link
Contributor Author

qcdyx commented Jan 24, 2024

@emmambd I have the route name deletion code locally. I'll commit it in a new PR together with GtfsNetworkSchema.

Copy link
Contributor

@jcpitre jcpitre left a comment

Choose a reason for hiding this comment

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

LGTM!

@jcpitre jcpitre merged commit 15df3ee into master Jan 24, 2024
333 checks passed
@jcpitre jcpitre deleted the 1640-add-remaining-gtfs-features-to-the-validator branch January 24, 2024 19:01
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.

Add remaining GTFS Features to the Validator
4 participants