-
Notifications
You must be signed in to change notification settings - Fork 100
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: verify conditional requirement of levels.txt presence #966
Conversation
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.
Great job! A single question in-line before approving :)
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/LevelPresenceValidator.java
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/LevelPresenceValidator.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Maxime Armstrong <[email protected]>
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.
Excellent 🎉
@lionel-nj can you rename this notice as "MissingLevelsFileNotice" to be consistent with MissingCalendarAndCalendarDateFilesNotice? |
Will do!
…On Mon, Aug 23, 2021 at 3:04 PM isabelle-dr ***@***.***> wrote:
@lionel-nj <https://github.com/lionel-nj> can you rename this notice as
"MissingLevelsFileNotice" to be consistent with
MissingCalendarAndCalendarDateFilesNotice?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#966 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIQXL7RKD7CBTBNZYOFPVOTT6KLVRANCNFSM5CMSIYKQ>
.
--
—
Lionel Nébot Janvier | fr, en, es | Software Developer and Mobility Expert
| MobilityData IO | mobilitydata.org
***@***.*** | Montréal, Québec, Canada
|
Should this be set at an error severity to be consistent with the spec and the other missing required file notices in the validator? |
Yes! |
if (levels.entityCount() != 0) { | ||
return; | ||
} | ||
ListMultimap<GtfsPathwayMode, GtfsPathway> byPathwayModeMap = ArrayListMultimap.create(); |
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.
What is the purpose of this map? Are you using it for any goal other than to check whether there is a pathway of mode=elevator
?
Did you have an intention to extend this code in the future?
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.
@aababilov As mentioned in #994 (comment), I used this as a way to avoid checking if each pathway has pathway mode set to elevator - we could remove it, but would need to do the additional check for each pathway.
Summary:
This PR provides support to verify conditionnal requirements on
levels.txt
.levels.txt
must be provided and non-empty if a row frompathways.txt
haspathways.pathway_mode=5
.Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anything[ ] Include screenshot(s) showing how this pull request works and fixes the issue(s)