locations.geojson POC phase2: End-to-end partial support of json data #1810
Conversation
jcpitre
added
the
do not merge
|
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 15f1fa2 📊 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
changed the title
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit e989e06 📊 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
changed the title
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit a5d8762 📊 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.
|
There was a problem hiding this comment.
Could you add class comments to help understanding the purpose of this abstract class?
There was a problem hiding this comment.
Added class comments.
There was a problem hiding this comment.
Same here, class comment could be useful.
There was a problem hiding this comment.
Added class comments.
There was a problem hiding this comment.
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.
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 ffecd98 📊 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.
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 0d8ddb7 📊 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 I tested duplicate_key with this PR using It worked! But I realized that we don't have the csvRowNumber lines...because it's not a csv. Do we have a plan or need to discuss how to modify these rowNumber columns for generic notices that can affect either a geojson file or a csv file? cc @davidgamez |
|
@emmambd Try to break/generate other generic notices since we weren't expecting |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 9912fdd 📊 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.
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 3900014 📊 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.
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit d62053f 📊 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.
|
| import org.mobilitydata.gtfsvalidator.validator.SingleEntityValidator; | ||
| import org.mobilitydata.gtfsvalidator.validator.ValidatorProvider; | ||
| import org.mobilitydata.gtfsvalidator.validator.ValidatorUtil; | ||
|
|
||
| public final class AnyTableLoader { | ||
| /** This class loads csv files specifically. */ | ||
| public final class AnyTableLoader extends TableLoader { |
There was a problem hiding this comment.
[suggestion]: rename this class as CsvFileLoader and the parent FileLoader. The loader for JSON files is already named JsonFileLoader.
|
|
||
| /** | ||
| * This class is the parent of the containers holding table (csv) entities and containers holding | ||
| * geojson entities |
There was a problem hiding this comment.
[suggestion]: change this term to a more generic one:
| * geojson entities | |
| * JSON entities |
|
|
||
| /** | ||
| * Container for a whole parsed GTFS feed with all its tables. | ||
| * | ||
| * <p>The tables are kept as {@code GtfsTableContainer} instances. | ||
| * <p>The tables are kept as {@code GtfsContainer} instances. |
There was a problem hiding this comment.
[doc]:
| * <p>The tables are kept as {@code GtfsContainer} instances. | |
| * <p>The tables are kept as {@code GtfsEntityContainer} instances. |
| @@ -7,35 +7,18 @@ | |||
| import org.mobilitydata.gtfsvalidator.notice.NoticeContainer; | |||
| import org.mobilitydata.gtfsvalidator.parsing.CsvHeader; | |||
|
|
|||
| public abstract class GtfsTableDescriptor<T extends GtfsEntity> { | |||
| public abstract class GtfsTableDescriptor<T extends GtfsEntity> extends GtfsFileDescriptor<T> { | |||
There was a problem hiding this comment.
[suggestion]: rename this class to GtfsCsvFileDescriptor.
There was a problem hiding this comment.
To be logical all TableDescriptor (e.g. CurrentcyAmountTableDescriptor) should also be renamed, right?
There was a problem hiding this comment.
This is becoming a massive change. To do things properly I would need to change any reference from Table to File. (e.g. TableLoader becomes FileLoader).
I am thinking of not doing this and having a dedicated PR for that change.
| severity = ERROR, | ||
| files = @GtfsValidationNotice.FileRefs(GtfsLocationsSchema.class), | ||
| sections = @SectionRefs(FILE_REQUIREMENTS)) | ||
| public class GeojsonDuplicateKeyNotice extends ValidationNotice { |
There was a problem hiding this comment.
As discussed, we can create this notice more generic renaming it to `JsonDuplicateKeyNotice, adding it the field name, field value and removing references to the featureId.
| import org.mobilitydata.gtfsvalidator.notice.NoticeContainer; | ||
| import org.mobilitydata.gtfsvalidator.validator.ValidatorProvider; | ||
|
|
||
| public class JsonFileLoader extends TableLoader { |
There was a problem hiding this comment.
This looks no generic enough to name it as JsonFileLoader. I suggest to rename it to GeoJsonFileLoader or make the implementation generic enough to cover any JSON descriptor.
| } catch (Exception e) { | ||
| System.out.println("Error: " + e); |
There was a problem hiding this comment.
👁️ Don't forget to revert these changes before merge if they were added to debug. If there are no other changes in this class other than formatting, I suggestion to revert all changes to reduce noise in this already large PR.
There was a problem hiding this comment.
This try catch was added by you. Was it only for debugging?
There was a problem hiding this comment.
Yes, it was due to an exception regarding the null tables, but that is not the case now.
| import org.mobilitydata.gtfsvalidator.validator.ValidatorLoaderException; | ||
|
|
||
| @RunWith(JUnit4.class) | ||
| public class CurrencyAmountSchemaTest { |
There was a problem hiding this comment.
👁️ revert these changes before merge.
| import org.mobilitydata.gtfsvalidator.validator.ValidatorLoaderException; | ||
|
|
||
| @RunWith(Parameterized.class) | ||
| public class MixedCaseSchemaTest { |
There was a problem hiding this comment.
👁️ revert there changes before merge.
| table | ||
| .map(gtfsTableContainer -> loadUniqueCount(gtfsTableContainer, clazz, idExtractor)) | ||
| .orElse(0)); | ||
| table.map(GtfsContainer -> loadUniqueCount(GtfsContainer, clazz, idExtractor)).orElse(0)); |
There was a problem hiding this comment.
rename parameter to:
| table.map(GtfsContainer -> loadUniqueCount(GtfsContainer, clazz, idExtractor)).orElse(0)); | |
| table.map(gtfsContainer -> loadUniqueCount(gtfsContainer, clazz, idExtractor)).orElse(0)); |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit a3e5685 📊 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.
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 7ec218a 📊 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.
|
|
@emmambd This PR is ready for a bit of QA |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit f61fb63 📊 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.
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 4ec1416 📊 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:
Geojsonin 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.geojsonis 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 testto make sure you didn't break anything