feat: 1822 flex forbidden pickup type & forbidden drop off type #1936
Conversation
qcdyx
changed the title
|
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: |
| entity.endPickupDropOffWindow())); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
You should maybe add a shouldValidate method that allows skipping the validate call as an optimisation.
See for example:
StopTimesShapeDistTraveledPresenceValidator.java
And here is the PR where this feature was added: #1875
|
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 b639c9a 📊 Notices ComparisonNew Errors (2 out of 1788 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Errors (0 out of 1788 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1788 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1788 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1788 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
| @Override | ||
| public void validate(GtfsStopTime entity, NoticeContainer noticeContainer) { | ||
| if ((entity.hasStartPickupDropOffWindow() || entity.hasEndPickupDropOffWindow()) | ||
| && (entity.pickupType().equals(GtfsPickupDropOff.ALLOWED) |
There was a problem hiding this comment.
I find it confusing that the regular pickup or dropoff enum value is ALLOWED when with this new rule it's actually not allowed.
Out of scope for this PR, I guess, but can't we rename the enum value to REGULAR instead of ALLOWED. Looking at the GTFS reference, we have this for pickup_type. drop_off_type is similar:
Indicates pickup method. Valid options are:
0 or empty - Regularly scheduled pickup.
1 - No pickup available.
2 - Must phone agency to arrange pickup.
3 - Must coordinate with driver to arrange pickup.
which more in line with having a REGULAR enum vlue.
There was a problem hiding this comment.
Hey @emmambd shall we open a new issue for it?
There was a problem hiding this comment.
I'm confused why it can't be done within the scope of this issue - how often does this enum value recur across notices?
There was a problem hiding this comment.
It can be done within this issue. I just don't want @qcdyx to feel obliged to do it since it has no effect on the resolution of the current issue.
There was a problem hiding this comment.
It'll be implemented in another PR - no need to open a new issue.
There was a problem hiding this comment.
I evaluated this request, it's not just about replacing a string because GtfsPickupDropOff is generated code, we need to identify the source template/code generation logic that produces the file. It's worthy of a new issue. #1942 @davidgamez
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/PickupDropOffTypeValidator.java
Outdated
Show resolved
Hide resolved
📝 Acceptance Test Report📋 Summary✅ The rule acceptance has passed for commit cc02367 📊 Notices ComparisonNew Errors (2 out of 1788 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Errors (0 out of 1788 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1788 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1788 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1788 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 5130474 📊 Notices ComparisonNew Errors (2 out of 1788 datasets, ~0%) ✅Details of new errors due to code change, which is less than the provided threshold of 1%.
Dropped Errors (0 out of 1788 datasets, ~0%) ✅No changes were detected due to the code change. New Warnings (0 out of 1788 datasets, ~0%) ✅No changes were detected due to the code change. Dropped Warnings (0 out of 1788 datasets, ~0%) ✅No changes were detected due to the code change. 🛡️ Corruption Check0 out of 1788 sources (~0 %) are corrupted. ⏱️ Performance Assessment📈 Validation TimeAssess the performance in terms of seconds taken for the validation process.
📜 Memory Consumption
|
|
Notice name, fields displayed, and description LGTM! |
Summary:
Closes #1822
Closes #1818
Expected behavior:
Display forbidden pickup/ drop off notice if conditions met.
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle testto make sure you didn't break anything