-
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
feat: 1814 flex forbidden continuous pickup or drop off #1941
feat: 1814 flex forbidden continuous pickup or drop off #1941
Conversation
…github.com:MobilityData/gtfs-validator into 1814-flex-forbidden_-continuous_pickup_or_drop_off
📝 Acceptance Test Report📋 Summary❌ The rule acceptance test has failed for commit f0f1055 📊 Notices ComparisonNew Errors (50 out of 1792 datasets, ~3%) ❌Details of new errors due to code change, which is above the provided threshold of 1%.
Dropped Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1792 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
Suggestion - Notice name: continuous_pickup_drop_off -> forbidden_continuous_pickup_drop_off Description: |
@tzujenchanmbd Please take a look and feel free to approve the PR. |
📝 Acceptance Test Report📋 Summary❌ The rule acceptance test has failed for commit 134191a 📊 Notices ComparisonNew Errors (50 out of 1792 datasets, ~3%) ❌Details of new errors due to code change, which is above the provided threshold of 1%.
Dropped Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1792 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 test has failed for commit 840490e 📊 Notices ComparisonNew Errors (50 out of 1792 datasets, ~3%) ❌Details of new errors due to code change, which is above the provided threshold of 1%.
Dropped Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1792 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
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.
Notice name, fields displayed, and description lgtm!
@tzujenchanmbd I find the number of feeds that trigger this error to be surprising - it's high. Is that expected from your perspective? |
Let me have a look before merging |
@emmambd thanks for catching this! Checked a couple of feeds that seems to trigger false positive error? such as: All |
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: |
Hello @tzujenchanmbd, I noticed that when both values are set to 1, the logic still passes the condition <routes.continuous_pickup OR routes.continuous_drop_off are NOT 1 or empty>. To address this, I updated the logic to ERROR: like what @emmambd suggested as the description of the notice. I tested the otterexpress dataset, the validation result looks good. |
...src/main/java/org/mobilitydata/gtfsvalidator/validator/ContinuousPickupDropOffValidator.java
Show resolved
Hide resolved
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 7dcb169 📊 Notices ComparisonNew Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1792 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 75bb490 📊 Notices ComparisonNew Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1792 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
...src/main/java/org/mobilitydata/gtfsvalidator/validator/ContinuousPickupDropOffValidator.java
Show resolved
Hide resolved
boolean continuous = | ||
(route.continuousPickup() == GtfsContinuousPickupDropOff.MUST_PHONE | ||
|| route.continuousPickup() == GtfsContinuousPickupDropOff.ON_REQUEST_TO_DRIVER) | ||
|| (route.continuousPickup() == GtfsContinuousPickupDropOff.MUST_PHONE |
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.
Looks like duplicate checks for route.continuousPickup()
?
Perhaps route.continuousDropOff()
instead of route.continuousPickup()
here?
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.
Done.
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit 457aff0 📊 Notices ComparisonNew Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1792 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
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 5d08853 📊 Notices ComparisonNew Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1792 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 5399f64 📊 Notices ComparisonNew Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1792 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1792 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
[suggestion]: The potential cause of the notice is not just in the |
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit d38a6ed 📊 Notices ComparisonNew Errors (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1801 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
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 4bb4dd7 📊 Notices ComparisonNew Errors (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Errors (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1801 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1801 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
Summary:
Closes #1814
Expected behavior:
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything