Skip to content

Conversation

@drozycki
Copy link
Contributor

@drozycki drozycki commented Aug 8, 2024

The new NextBus/UmoIQ API supports URLs with the route_id segment omitted, like api/pub/v1/agencies/sfmta-cis/stops/7999/predictions. This is useful when multiple complementary routes use the same stop, such as the SFMTA N (light rail sharing traffic lane with cars and buses), NBUS (alternate bus service), and NOWL (overnight bus service).

This PR extends the predictions_for_stop API by making the route_id param optional. I have tested this both with and without the parameter provided. I have also bumped the version.

I would like to integrate this functionality into the ha nextbus integration so that a single entity can optionally represent the combined predictions for all routes at a stop.

@ViViDboarder
Copy link
Owner

This is an interesting change that I'd like to consider, but I just updated the method to perform the filter that you proposed in the hass/core repo. Can you rebase and add this again? Coupled with a refactor in the HASS component, the coordinator can more efficiently request predictions using this method and filtering results in HASS.

@drozycki
Copy link
Contributor Author

Your unfiltered change is interesting, although I would be concerned that the route/stop/predictions API could be fixed in the future and break any code that relies on its current behavior with unfiltered = True. The route/predictions API may be more stable (although nothing is guaranteed with an undocumented API anyways..) I rebased my changes on top of yours and made sure that the unfiltered response is always returned when route is omitted. I manually tested my changes in the python interactive interpreter, and the test suite passes as well.

@ViViDboarder
Copy link
Owner

Since Home Assistant is really the only known use case, I could probably just remove it. I figured I’d try to cover bases to provide something backwards compatible, but I doubt there’s a good reason unless you don’t provide a route in the first place.

Copy link
Owner

@ViViDboarder ViViDboarder left a comment

Choose a reason for hiding this comment

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

Are tests green? It might be good to add a test that verifies the resulting API call and that filtering is skipped. I only have tests for this method because the others basically just return the JSON result as received.

@drozycki
Copy link
Contributor Author

drozycki commented Aug 19, 2024

Added test, all tests green, removed redundant unfiltered option

@ViViDboarder
Copy link
Owner

I just manually merged this because pre-commit hooks were failing. Thanks!

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