-
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: Date validity - Calendar Expiration + Trip Coverage #1289
feat: Date validity - Calendar Expiration + Trip Coverage #1289
Conversation
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/CalendarValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/CalendarValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/CalendarValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/CalendarValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/DateTripsValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/DateTripsValidator.java
Outdated
Show resolved
Hide resolved
|
09c1755
to
ff5ba70
Compare
Apologies for the slow delay. So looking at the logic for trip counting, I realize there is some additional complexity around handling things like frequency-based trips that the current code, as written, doesn't cover. Digging into this on my side, I've gotten the go-ahead to open-source some utility code we have at Google that might help with this. @briandonahue if it's ok with you, can I send you a PR with that code and talk about how it might simplify this PR? Will send shortly. |
…s and service windows.
@bdferris-v2 this has now been updated to use |
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/DateTripsValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/DateTripsValidator.java
Outdated
Show resolved
Hide resolved
main/src/main/java/org/mobilitydata/gtfsvalidator/validator/DateTripsValidator.java
Show resolved
Hide resolved
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.
Thank you for the work here @briandonahue, and thank you for the review @bdferris-v2!
To follow our naming conventions, could you rename trip_data_should_be_valid_for_next7_days
with trip_expiration7_days
?
Also: what conditions are triggering the expired_calendar
notice exactly? Do you have a test dataset for it?
Naming conventions aside, I'm not sure |
@isabelle-dr I didn't put a wide variety of test data into the tests because the new validators rely on I haven't come across any examples of using mocks in the tests (very possible I've just missed them, if they exist) but testing this might be more clear if the utils we're mocked and just return specific window dates, rather than input test data and rely on the actual implementations of the Util classes in these tests. |
@briandonahue what I meant is more: what is the pseudo-code here? The name: I see Brian's point; for the purposes of making this name relatively short, I suggest: |
@isabelle-dr The I can try to come up with an English translation for the documentation - it looks like it looks for consecutive days with > 75% of trips to calculate the service window. Open to suggestion for wording or any existing wording perhaps used by Google, @bdferris-v2? |
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.
Hello @briandonahue I've added a suggestion in the doc to be a little more descriptive about how this role works.
Besides this small change, we are ready to merge!
Co-authored-by: isabelle-dr <isabelle@mobilitydata.org>
@briandonahue can you update this branch with master? I will merge after |
@isabelle-dr sorry, lost track of this! Done! |
…ta#1289) * feat: date validity * Correct typo in test name * fix: dates in notice * test: basic tests for date trips validator * Add documentation, incorporate feedback * Include calendar date exceptions in expired calendar check * refactor & cleanup * formatting * Update warning information. * Add TripCalendarUtil, which provides methods for computing trip counts and service windows. * Update to use TripCalendarUtil for date calculations * remove unused code * bug fix and var type clarification * Add doc comments for new Notices * Fix comment to match other examples * formatting * Rename notice, fix failing tests with notice fields * Update RULES.md Co-authored-by: isabelle-dr <isabelle@mobilitydata.org> --------- Co-authored-by: Brian Donahue <brian.donahue@vitreosolutions.com> Co-authored-by: Brian Ferris <bdferris+v2@google.com> Co-authored-by: isabelle-dr <isabelle@mobilitydata.org>
Summary:
Resolves #886
Expected behavior:
Explain and/or show screenshots for how you expect the pull request to work in your testing (in case other devices exhibit different behavior).
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle test
to make sure you didn't break anythingNote: Marking as draft until I complete unit tests for the new validator.