feat: 1980 add fare product with multiple default rider categories #1998
Conversation
…t_rider_categories
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 0317690 📊 Notices ComparisonNew Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (1 out of 1824 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
New Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1824 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
…t_rider_categories
|
Looks like the missing_required_column issue is still happening with acceptance tests. cc @davidgamez |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 67a8ef2 📊 Notices ComparisonNew Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1824 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
|
@qcdyx I'm not sure about the tests, the test is passing but I feel like it's not validating the proposed behavior. I also added a comment about the creation of test rider_categories. With that one, the rider_category_id is unique. So one category cannot be default and not default at the same time |
|
@emmambd @davidgamez Could you clarify the next steps for this rule?
|
|
@qcdyx what I meant was the tests are probably not testing what the notice should flag. The notice code looks right (my java isn't great but it seems like you're createing a map of productId -> riderCategoryId if it is default then counting the occurences). |
🚨 Code Formatting Issue 🚨This contribution does not follow the conventions set by the Google Java Style Guide. Please run the following command at the root of the project to fix formatting errors: ./gradlew spotlessApply🗂️ Affected files
Go to the Actions Dashboard to download the full Spotless output |
…t_rider_categories
🚨 Code Formatting Issue 🚨This contribution does not follow the conventions set by the Google Java Style Guide. Please run the following command at the root of the project to fix formatting errors: ./gradlew spotlessApply🗂️ Affected files
Go to the Actions Dashboard to download the full Spotless output |
🚨 Code Formatting Issue 🚨This contribution does not follow the conventions set by the Google Java Style Guide. Please run the following command at the root of the project to fix formatting errors: ./gradlew spotlessApply🗂️ Affected files
Go to the Actions Dashboard to download the full Spotless output |
…ories' of github.com:MobilityData/gtfs-validator into 1980-add-fare_product_with_multiple_default_rider_categories
…t_rider_categories
🚨 Code Formatting Issue 🚨This contribution does not follow the conventions set by the Google Java Style Guide. Please run the following command at the root of the project to fix formatting errors: ./gradlew spotlessApply🗂️ Affected files
Go to the Actions Dashboard to download the full Spotless output |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit f36d6b2 📊 Notices ComparisonNew Errors (3 out of 1824 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1824 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
|
Waiting on @skalexch for this |
…t_rider_categories
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 67855c4 📊 Notices ComparisonNew Errors (3 out of 1824 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Errors (1 out of 1824 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
New Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1824 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
|
@qcdyx thanks for updating. There's one detail left to clear out the false positives. |
…t_rider_categories
…ories' of github.com:MobilityData/gtfs-validator into 1980-add-fare_product_with_multiple_default_rider_categories
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit d89e2a6 📊 Notices ComparisonNew Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1824 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
Hi @skalexch, does this require clarification on the spec? |
|
@davidgamez maybe we can change the description to "When multiple unique rider categories are eligible for a single fare product specified by a fare_product_id, there must be exactly one of these eligible rider categories indicated as the default rider category (is_default_fare_category = 1)." |
|
@skalexch You mean on the spec side? Sounds good. The validator side will be merged this week and we can make changes later post 7.0 |
|
@emmambd yes on the spec side. On the validator side once Jingsi resolves the final comment I'll do one final round of QA and I think it will be ready to go. |
My only concern is that is not clear that
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 08259ca 📊 Notices ComparisonNew Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1824 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
🚨 Code Formatting Issue 🚨This contribution does not follow the conventions set by the Google Java Style Guide. Please run the following command at the root of the project to fix formatting errors: ./gradlew spotlessApply🗂️ Affected files
Go to the Actions Dashboard to download the full Spotless output |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 34c9601 📊 Notices ComparisonNew Errors (3 out of 1824 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1824 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit fe10753 📊 Notices ComparisonNew Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1824 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
skalexch
left a comment
skalexch
left a comment
There was a problem hiding this comment.
Looks good! Thanks for the work!
| static List<GtfsFareProduct> createFareProductTable() { | ||
| List<GtfsFareProduct> fareProducts = new ArrayList<>(); | ||
| fareProducts.add(createFareProduct(1, "fare1", "rider1")); | ||
| fareProducts.add(createFareProduct(2, "fare2", "rider2")); |
There was a problem hiding this comment.
I'm not sure if the test validates a correct behavior
For fare products, the fare_product_id should be the same
fareProducts.add(createFareProduct(1, "fare1", "rider1"));
fareProducts.add(createFareProduct(1, "fare1", "rider2"));
| static List<GtfsRiderCategories> createRiderCategoriesTable() { | ||
| List<GtfsRiderCategories> riderCategories = new ArrayList<>(); | ||
| riderCategories.add(createRiderCategories(1, "rider1", GtfsRiderFareCategory.IS_DEFAULT)); | ||
| riderCategories.add(createRiderCategories(2, "rider1", GtfsRiderFareCategory.NOT_DEFAULT)); |
There was a problem hiding this comment.
Just for spec correctness and to work with the suggested change above, we cannot have the same rider_category_id associated with Default and Not default.
I think a more compliant approach is
riderCategories.add(createRiderCategories(1, "rider1", GtfsRiderFareCategory.IS_DEFAULT));
riderCategories.add(createRiderCategories(2, "rider2", GtfsRiderFareCategory.IS_DEFAULT));
riderCategories.add(createRiderCategories(3, "rider3", GtfsRiderFareCategory.IS_NOT_DEFAULT));
qcdyx
deleted the
1980-add-fare_product_with_multiple_default_rider_categories
branch
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 6b225a7 📊 Notices ComparisonNew Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1824 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1824 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
Summary:
Closes #1980
Expected behavior:
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle testto make sure you didn't break anything