Add Grandstream Home integration#167872
Conversation
d30e08d to
fef1454
Compare
8aad180 to
39a400b
Compare
|
The source for the library is missing: https://github.com/GrandstreamEngineering/grandstream_home_api |
f443b7c to
aa77908
Compare
It is added now |
aa77908 to
8a3b791
Compare
8a3b791 to
0b3e604
Compare
edenhaus
left a comment
There was a problem hiding this comment.
I did an initial review until I stopped as this PR is to huge. Some code needs to move to the library and also the config flow is huge. Please refactor the code and make it more readable.
Also the library needs a working workflow to deploy directly to pypi.
As 10.000 changes is to much, can we just add a single device/model in this PR and then add other models in a follow up PR. This PR needs to be a lot smaller
| unique-config-entry: done | ||
|
|
||
| # Silver | ||
| action-exceptions: done |
There was a problem hiding this comment.
Please set all rules except the bronze one to todo. As they need to be evaluated after the integration was added
| async def _handle_existing_device( | ||
| hass: HomeAssistant, unique_id: str, name: str, device_type: str | ||
| ) -> None: | ||
| """Check and update existing device if found.""" | ||
| device_registry = dr.async_get(hass) | ||
|
|
||
| for dev in device_registry.devices.values(): | ||
| for identifier in dev.identifiers: | ||
| if identifier[0] == DOMAIN and identifier[1] == unique_id: | ||
| _LOGGER.info("Found existing device: %s, name: %s", dev.id, dev.name) | ||
|
|
||
| # Update device attributes | ||
| device_registry.async_update_device( | ||
| dev.id, | ||
| name=name, | ||
| manufacturer="Grandstream", | ||
| model=device_type, | ||
| ) | ||
| return |
| api = await _setup_api_with_error_handling(hass, entry, device_type) | ||
|
|
||
| # Store API in runtime_data (required for Bronze quality scale) | ||
| entry.runtime_data = {"api": api} |
There was a problem hiding this comment.
Use a typed classed not a dictionary
| hass.data.setdefault(DOMAIN, {}) | ||
| hass.data[DOMAIN][entry.entry_id] = {} |
There was a problem hiding this comment.
| hass.data.setdefault(DOMAIN, {}) | |
| hass.data[DOMAIN][entry.entry_id] = {} |
Use runtime data instead
| raise # Let auth failures propagate to trigger reauth flow | ||
| except Exception as e: | ||
| _LOGGER.exception("Error setting up integration") | ||
| raise ConfigEntryNotReady("Integration setup failed") from e |
There was a problem hiding this comment.
We should not put all code into the try catch block. Just put the code in it, which is also raising the specific exceptions
| return self.device_model | ||
| return self.device_type or "Unknown" | ||
|
|
||
| def _register_device(self) -> None: |
There was a problem hiding this comment.
Why are you needing this?
device_info is in the most cases enough as the device will be created and also updated. Just make sure to have a unique identifier
| """Custom exceptions for Grandstream Home integration - re-exported from library.""" | ||
|
|
||
| from grandstream_home_api.error import ( | ||
| GrandstreamError, | ||
| GrandstreamHAControlDisabledError, | ||
| ) | ||
|
|
||
| __all__ = [ | ||
| "GrandstreamError", | ||
| "GrandstreamHAControlDisabledError", | ||
| ] |
There was a problem hiding this comment.
| """Custom exceptions for Grandstream Home integration - re-exported from library.""" | |
| from grandstream_home_api.error import ( | |
| GrandstreamError, | |
| GrandstreamHAControlDisabledError, | |
| ) | |
| __all__ = [ | |
| "GrandstreamError", | |
| "GrandstreamHAControlDisabledError", | |
| ] |
Makes no sense
| { | ||
| "domain": "grandstream_home", | ||
| "name": "Grandstream Home", | ||
| "codeowners": ["@GrandstreamEngineering"], |
There was a problem hiding this comment.
You should be the code owner and not a random user. If you want to add them too, then they need to comment on this PR, thats it's fine for them to be code owner
There was a problem hiding this comment.
GrandstreamEngineering is an organization
| "name": "Grandstream Home", | ||
| "codeowners": ["@GrandstreamEngineering"], | ||
| "config_flow": true, | ||
| "dependencies": [], |
| ) | ||
|
|
||
| @staticmethod | ||
| def _get_by_path(data: dict[str, Any], path: str, index: int | None = None): |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
This integration adds support for Grandstream GDS/GNS/GSC devices.
Features:
Breaking change
Proposed change
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: