-
Notifications
You must be signed in to change notification settings - Fork 3
feat: New get_endpoints function for alert endpoints in Subway Status
#2822
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
Conversation
deanshi
left a comment
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.
Still parsing some of the PolyTree stuff and comparing with the dotcom work, but some small questions in the meantime.
| nil | ||
| else | ||
| stop_sequence | ||
| |> Enum.filter(fn {id, _} -> Enum.member?(ie_stops, id) end) |
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.
Do we need this filter check?
I feel like I'm just missing an edge case but are there cases where the Alert itself, after filtering for the route, will have stops that don't exist in stop_sequence (which to my understanding is just a huge map of all of the stops to being with).
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.
Good question! The reason it's structured this way is because there are IDs in ie_stops that aren't associated with a boarding location. This is encoded in location_type of the stop, but we don't get that information from the API within the informed_entities of an alert. Starting with the stop_sequence and then narrowing down to IDs included in the alert is a way to make sure that we only include the boarding locations.
Looking back at this function it is a bit confusing, so I will at least add a comment here the next time I'm updating it before merging.
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.
This is encoded in
location_typeof the stop, but we don't get that information from the API within theinformed_entitiesof an alert.
Drive-by comment, but we very much could if we wanted to! We already do something like this for Facilities; you'll note the facility field of an informed entity is the entire Facility struct, not just a facility ID. The reason this isn't a trivial change is because of all the existing places in the code that assume the stop field is only a stop ID rather than a struct, and tracking down all these places is tricky because there is no proper InformedEntity struct that would be legible to the LSP/compiler, they're just anonymous maps (there is also no good reason for this, and I strongly believe it should change).
As always, there is not strictly a need to address this here. But I think this is maybe the third or fourth time this exact issue has come up within the past year, so I'm feeling like it should at least be translated into a refactor task to be discussed and estimated in Asana.
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.
Agreed, it seems like this comes up almost every time we are adding an alert related feature. I created an Asana task for that refactor we could discuss tomorrow
deanshi
left a comment
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.
Double checked on dev-green and it's looking good.
Asana task: Update Subway Status to deal with GL combined stops being closed
Creates a new
get_endpointsfunction used by thesubway_statuswidget to determine the names for the endpoints of alerts.UnrootedPolytreedata structureget_endpointsfunction as well