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

Add CityBus integration #129231

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

ericswpark
Copy link

@ericswpark ericswpark commented Oct 26, 2024

Proposed change

This integration is based off of the NextBus integration and adds support for the CityBus API.
CityBus is a bus service that operates in Lafayette, Indiana.
This integration makes it possible for HA users to see upcoming bus times for the different route/direction/stop combinations on CityBus.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @ericswpark

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

CityBus switches around the keys periodically, breaking the integration.
Store the name of the route and direction destination, and perform key
lookup on fetching sensor data.
@joostlek
Copy link
Member

Okay 4 things:

  1. This is just copy pasted from Nextbus, do you know how everything works?
  2. Please setup a proper development environment
  3. We require full unit test coverage for the config flow
  4. What's the technical difference between nextbus and citybus? Because if there is none, should we just try to add citybus as a feature to nextbus?

@joostlek joostlek marked this pull request as draft October 28, 2024 14:02
@ericswpark
Copy link
Author

ericswpark commented Oct 28, 2024

@joostlek

1. This is just copy pasted from Nextbus, do you know how everything works?

It's not a copy-paste. The way we retrieve and display bus information for a given route on CityBus is rather different than how Nextbus does it. I referred to the Nextbus code as a starting point, but each line has been adapted for the CityBus integration.

2. Please setup a proper development environment

Will do -- I'm currently using Windows so tested the integration with HACS. Will get a proper environment running with WSL or a Linux VM/partition later this week.

3. We require full unit test coverage for the config flow

Where would I add this? I didn't see such test coverage for Nextbus.

4. What's the technical difference between nextbus and citybus? Because if there is none, should we just try to add citybus as a feature to nextbus?

You can't add CityBus as a "feature" to Nextbus. They use completely different API endpoints and schemas. The configuration alone requires different config options compared to what Nextbus asks.

@ericswpark
Copy link
Author

@joostlek please let me know if I can mark this PR as ready for review, or if there are any other changes I should make. I can't seem to find any documentation on adding unit test coverage.

@joostlek
Copy link
Member

There is documentation about code coverage. Nextbus has code coverage. Can't link it at the moment

@cgarwood
Copy link
Member

Here's the config flow tests from Nextbus, you may be able to adapt it for this integration.
https://github.com/home-assistant/core/blob/dev/tests/components/nextbus/test_config_flow.py

There's some general documentation on tests at https://developers.home-assistant.io/docs/development_testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants