feat: validate stair_count, traversal_time and max_slope against pathway_mode - #2170
feat: validate stair_count, traversal_time and max_slope against pathway_mode#2170vssinghh wants to merge 3 commits into
Conversation
The GTFS reference states that pathways.stair_count is recommended for stairs (pathway_mode=2). Nothing checked this, so feeds could omit the step count on stairs without any signal. Adds PathwayStairCountValidator, which emits the existing MissingRecommendedFieldNotice when a pathway has pathway_mode=2 and no stair_count. This follows the same shape as FareMediaNameValidator and RouteAgencyIdValidator, which raise the generic recommended-field notice under a conditional rather than defining a new notice type. Closes MobilityData#1788
|
Thanks for opening this pull request! You're awesome. We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of titles with semantic prefixes:
|
skalexch
left a comment
skalexch
left a comment
There was a problem hiding this comment.
@vssinghh thank you for this addition! The logic looks correct along with the tests.
One question: Do you have a capacity to do the same for traversal_time and max_slope?
traversal_time is recommended for pathway_modes=3, 4 and 5 (missing_recommended_field). Why max_slope should only be used for pathway_modes=1 and 3 (maybe a new rule like irrelevent_max_slope_set_for_pathway_mode).
Let me know if you have the capacity to do them both or at least one of them. You could do it in the same file and rename the file.
|
Thank you for the comment. I will take those as well. |
Extends the stair_count rule to the other two pathway fields whose expectations depend on pathway_mode, as requested in review: - traversal_time is recommended for pathway_mode 3, 4 and 5, reported with the existing missing_recommended_field notice - max_slope should only be used with pathway_mode 1 and 3, reported with a new irrelevant_max_slope_set_for_pathway_mode warning A max_slope of 0 is not reported, since the spec defines both an empty value and 0 as "no slope". Renames PathwayStairCountValidator to PathwayModeFieldsValidator. Closes MobilityData#1789
vssinghh
changed the title
|
Updated the files. |
|
@vssinghh thanks for the change! I tested on a deliberately incorrect pathways file and it worked! (check screenshot) One small thing that I forgot (my apologies), if you could add the missing_recommended_field notice to Otherwise, everything looks great! I'll approve right after. 
|
Completes the set of pathway fields whose expectations depend on pathway_mode. The spec recommends length for walkways (pathway_mode=1), fare gates (pathway_mode=6) and exit gates (pathway_mode=7), reported with the existing missing_recommended_field notice. Unlike max_slope, the spec gives no special meaning to a length of 0, so a zero value counts as defined and is not reported. Closes MobilityData#1787
|
Thank you for the comment. Added it. |
skalexch
left a comment
skalexch
left a comment
There was a problem hiding this comment.
Approved on a spec side. I'll wait on a dev person to go through the code and then you're clear. @davidgamez @jcpitre @cka-y
3 participants
Summary:
Adds
PathwayModeFieldsValidator, covering the four pathway fields whose expectations depend onpathway_mode:lengthmissing_recommended_fieldstair_countmissing_recommended_fieldtraversal_timemissing_recommended_fieldmax_slopeirrelevant_max_slope_set_for_pathway_mode(new, WARNING)Closes #1787, #1788 and #1789.
A
max_slopeof0is not reported, since the spec defines both an empty value and0as "no slope". Alengthof0is reported as defined, since the spec gives it no special meaning.Expected behavior:
Running the CLI on the
Archive.zipfrom #1788 gives 9 warnings:max_slopegets no coverage from that sample because the file has nomax_slopecolumn. Unit tests cover it, 15 in total across the four rules.gradle testto make sure you didn't break anything