Skip to content
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: Expand stops filters to include routes at stops #2131

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

sloanelybutsurely
Copy link
Contributor

Adds fetch_routes_for_stops/1 and uses that to expand a stops: filter similar to the V3 API.

Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have missed or misunderstood something, but I was under the impression from previous discussions that re-implementing the API's "alert matching" logic within our cache was specifically something we wanted to avoid — with the reasoning that, in most cases where we fetch alerts, we already have the needed context of route type/route/stop/direction handy, and could pass those as explicit filters to the cache functions (which would be taken literally, not "expanded"). I don't disagree with this approach, just wondering how we arrived here.

EDIT: Taking the approach of being as compatible as possible with how the V3 API works, so we have greater confidence we're not changing the behavior of any existing uses of alert fetching in our app.

lib/screens/stops/stop.ex Outdated Show resolved Hide resolved
test/screens/alerts/alert_test.exs Outdated Show resolved Hide resolved
@sloanelybutsurely sloanelybutsurely force-pushed the sloane/push-nqtoonvunxwn branch 2 times, most recently from 8042c3f to 3fb90a0 Compare August 15, 2024 21:21
@sloanelybutsurely sloanelybutsurely requested review from digitalcora and a team August 16, 2024 14:08
lib/screens/routes/route.ex Show resolved Hide resolved
lib/screens/alerts/cache/filter.ex Outdated Show resolved Hide resolved
Adds `fetch_routes_for_stops/1` and uses that to expand a `stops:`
filter similar to the V3 API.
Copy link
Contributor

@digitalcora digitalcora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚉 🔍 ✅

@sloanelybutsurely sloanelybutsurely merged commit bb01c30 into main Aug 16, 2024
11 checks passed
@sloanelybutsurely sloanelybutsurely deleted the sloane/push-nqtoonvunxwn branch August 16, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants