Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new capability check currently hard-codes http://{host} and drops the configured scheme/port from base_url, which can break HTTPS/custom-port configurations.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds a new Fritzhome.has_smarthome_capabilities() helper to detect whether a FRITZ!Box exposes smart home functionality by probing the TR-064 device description with a REST API fallback, along with tests and fixtures to validate the behavior.
Changes:
- Added
Fritzhome.has_smarthome_capabilities()to detect smart home support via TR-064 and REST description probing. - Added unit tests and new XML/JSON response fixtures for supported/unsupported and error/fallback scenarios.
- Extended
tests.Helper.response()to load fixtures by extension (XML/JSON).
File summaries
| File | Description |
|---|---|
pyfritzhome/fritzhome.py |
Adds has_smarthome_capabilities() implementation and supporting imports. |
tests/test_fritzhome.py |
Adds coverage for TR-064 detection, REST fallback, and error/parse-failure cases. |
tests/helper.py |
Updates fixture loader to support non-XML fixtures via an extension parameter. |
tests/responses/tr64desc.xml |
New TR-064 fixture including X_AVM-DE_Homeauto service. |
tests/responses/tr64desc_no_homeauto.xml |
New TR-064 fixture without X_AVM-DE_Homeauto service. |
tests/responses/rest_api_desc.json |
New REST description fixture including /smarthome endpoints. |
tests/responses/rest_api_desc_no_smarthome.json |
New REST description fixture without /smarthome endpoints. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
There is a concrete robustness issue in REST JSON shape handling (can raise AttributeError instead of returning None) and the encoding cookie placement is broken by the new blank line.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
pyfritzhome/fritzhome.py:112
json.loads(plain).get(...)and the subsequentendpoint.get(...)assume the REST API description is a dict with anendpointslist of dicts. If the response is valid JSON but has an unexpected shape (e.g., list at the top level), this will raiseAttributeErrorand break the method instead of returningNoneper the docstring.
pyfritzhome/fritzhome.py:5- The UTF-8 coding cookie must be on the first or second line (PEP 263). The newly added blank line pushes
# -*- coding: utf-8 -*-to line 3, so it will be ignored by the interpreter.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The REST fallback can misclassify capability as unsupported when "endpoints" is an iterable non-list (e.g., a dict), returning False instead of None for an indeterminate/invalid schema.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The added blank line at the top of pyfritzhome/fritzhome.py makes the UTF-8 encoding cookie invalid per PEP 263 and should be corrected.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pyfritzhome/fritzhome.py:4
- The encoding cookie must be on the first or second line (PEP 263). The added blank line pushes
# -*- coding: utf-8 -*-to line 3, which makes it ineffective (and can break Python 2 parsing if non-ASCII is ever introduced). Move the encoding cookie up to line 2 or remove it entirely.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches the stated behavior and the added tests/fixtures cover the expected success, fallback, and failure cases (including IPv6).
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
7410bdb to
74d2a58
Compare
Co-authored-by: Copilot copilot@github.com ensure correct IPv6 handling Co-authored-by: Copilot <copilot@github.com> fetch unexpected JSON response Co-authored-by: Copilot <copilot@github.com> apply code review
74d2a58 to
966130a
Compare
Add
has_smarthome_capabilities()toFritzhomeChecks whether a device supports smart home functionality: first via the TR-064
tr64desc.xmldescription (X_AVM-DE_Homeautoservice), falling back torest_api_desc.json(/smarthomeendpoint) if that fails. ReturnsTrue/False, orNoneif neither could be determined.Includes fixtures and tests for all paths (found, not found, fallback, failure).