-
Notifications
You must be signed in to change notification settings - Fork 101
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
locations.geojson POC phase2: End-to-end partial support of json data #1810
Conversation
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: |
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.
Could you add class comments to help understanding the purpose of this abstract class?
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.
Added class comments.
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.
Same here, class comment could be useful.
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.
Added class comments.
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.
What's the benefit of pulling TableStatus out from GtfsTableContainer? Just curious, coz nothing is changed in this enum class.
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.
I would say it's not essential but it's not bad to have a file dedicated to one enum.
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 4f9e3db 📊 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 Check0 out of 1575 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
|
@jcpitre One note re: the test notice of So there are two options:
|
Yes, I put anything related to location_groups.txt in comments in the validator class so we hopefully don't forget. |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 78eee27 📊 Notices ComparisonNew Errors (0 out of 1588 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1588 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1588 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1588 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1588 sources (~0 %) 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.
LGTM
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 3dcfc5c 📊 Notices ComparisonNew Errors (0 out of 1588 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1588 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1588 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1588 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1588 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
|
Summary:
Builds on the poc written by @davidgamez (#1805), related to locations.geojson
What it does:
Geojson
in the name)Not covered:
To be done:
This is NOT ready to merge yet.
I modified an existing dataset that has flex data to create an error where the id of a feature in
locations.geojson
is the same as an id instops.txt
.browncounty-mn-us--flex-v2-broken.zip
Here is a capture of the sample notice in the report:
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything