All committers have signed the CLA.

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ KClough
❌ briandonahue
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

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.

@bdferris-v2 this has now been updated to use TripCalendarUtil. Thanks for that!

Naming conventions aside, I'm not sure trip_expiration7_days is a particularly good name for what this notice is validating.
Specifically, it's not trips that are expiring (whatever that might mean). Maybe something like majority_of_trip_coverage_not_active_for_next7_days would be more descriptive and still match naming conventions?

Also: what conditions are triggering the expired_calendar notice exactly? Do you have a test dataset for it?

@isabelle-dr I didn't put a wide variety of test data into the tests because the new validators rely on CalendarUtil and TripCalendarUtil which are both tested for specific scenarios. So, my test data is fairly basic, just checking that it verifies when the start/end dates are not covering the 7 day range. For DateTripsValidator it's the same philosophy - I just verify that it validates the start/end dates of the majority service window as returned by TripCalendarUtil.

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?
Did you use the formula described in this comment? If so, can you update the documentation so users better understand what this notice means exactly and what needs to be changed in the data so that this notice is removed?

The name: I see Brian's point; for the purposes of making this name relatively short, I suggest: trip_coverage_not_active_for_next7_days

@briandonahue what I meant is more: what is the pseudo-code here?

@isabelle-dr The TripCalendarUtil was imported by @bdferris-v2 from Google projects, as far as I understand, and merged in #1351 so that I could use it here. The specific method being used to calculate "Majority Trip Service Coverage" is here.

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?

@briandonahue can you update this branch with master? I will merge after

@briandonahue can you update this branch with master? I will merge after

@isabelle-dr sorry, lost track of this! Done!