-
Notifications
You must be signed in to change notification settings - Fork 100
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: 1534 service window in summary report #1837
Conversation
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 039dbfa 📊 Notices ComparisonNew Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check7 out of 1575 sources (~0 %) are corrupted.
⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit a12fcf6 📊 Notices ComparisonNew Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check7 out of 1575 sources (~0 %) are corrupted.
⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
|
main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadata.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadata.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadata.java
Show resolved
Hide resolved
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: |
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: |
main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadata.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadata.java
Outdated
Show resolved
Hide resolved
main/src/test/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadataTest.java
Outdated
Show resolved
Hide resolved
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit ce2a0f1 📊 Notices ComparisonNew Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check7 out of 1575 sources (~0 %) are corrupted.
⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
|
main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadata.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadata.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/report/model/FeedMetadata.java
Outdated
Show resolved
Hide resolved
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit bd3a7a1 📊 Notices ComparisonNew Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check20 out of 1588 sources (~1 %) are corrupted.
⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @qcdyx! After doing some spot checks, it makes sense for us to include both the feed start and end date values and the service date range values.
Minor changes:
• Change "Service Date Range" to "Service Dates"
• Add tooltip to Service Dates on the HTML report:
"The range of service dates covered by the feed, based on trips with an associated service_id in calendar.txt and/or calendar_dates.txt."
@qcdyx Sorry Jingsi, one more minor requirements change and then you can merge 🙈 In thinking about how we'll eventually add majority service window to this report, let's change "service dates" to "service window" so we'll have consistency between these concepts. Once changed, all good and ready to merge :) |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit ce2d112 📊 Notices ComparisonNew Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check20 out of 1588 sources (~1 %) are corrupted.
⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
|
@qcdyx The acceptance tests are reporting a few(20) feeds sources After the changes, let's examine the corrupted files in the acceptance test and ensure that the validator generates the report. |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 9942afa 📊 Notices ComparisonNew Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1568 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check20 out of 1588 sources (~1 %) are corrupted.
⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
|
…bilityData/gtfs-validator into 1534-service-window-in-summary-report
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: |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 6118867 📊 Notices ComparisonNew Errors (0 out of 1575 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1575 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1575 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1575 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check13 out of 1588 sources (~1 %) are corrupted.
⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
|
This looks good for the scope of the issue. I did create a follow-up issue #1848 to address the pending corrupted feeds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Was the service window added to the json report? It's part of the tasking, but couldn't find the information from running the jar from this branch |
Correct me if I'm wrong but I remembered that we talked about splitting the ticket and focusing on just the service date range on UI when we did the estimate. I recommend merging this PR now, and I’ll create a separate one to add it to the JSON report coz I need to merge master changes into my branch and it takes some time to go thru all the checks. @emmambd @davidgamez |
I'm fine with splitting, but the next priority issue will be adding it to the JSON report in that case, since we can't make any changes to the Mobility Database we want without the JSON report functionality. cc @davidgamez for final call |
@emmambd could you approve my PR since merging is blocked due to requested changes. |
Did you and @davidgamez discuss? Just waiting for his thoughts on splitting or not before I approve it. |
Yes the JSON changes can be part of a different issue. |
Summary:
Closes #1534
Expected behavior:
GTFS Validator report should show Service Date Range under Feed Info section.
When only calendar_dates.txt is used
test dataset:
poa_gtfs.zip
)
When only calendar_dates.txt is used
test dataset:
Google_transit.zip
When both calendars.txt and calendar_dates.txt are used
test dataset:
Archive.zip
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything