Create, update and delete KNX entities from UI / WS-commands #104079
Conversation
|
Hey there @Julius2342, @marvin-w, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
farmio
force-pushed
the
knx-entity-creation
branch
2 times, most recently
from
8673b57 to
1736de7
Compare
farmio
force-pushed
the
knx-entity-creation
branch
from
1736de7 to
8a830f4
Compare
farmio
force-pushed
the
knx-entity-creation
branch
2 times, most recently
from
4477ac4 to
b852ba4
Compare
farmio
force-pushed
the
knx-entity-creation
branch
from
2549850 to
3d7de94
Compare
farmio
force-pushed
the
knx-entity-creation
branch
2 times, most recently
from
5db4065 to
3b74b5d
Compare
farmio
force-pushed
the
knx-entity-creation
branch
from
3b74b5d to
f0876a5
Compare
farmio
force-pushed
the
knx-entity-creation
branch
from
53f21d5 to
02cb264
Compare
farmio
changed the title
|
@coderabbitai review |
Actions PerformedReview triggered.
|
WalkthroughThe updates to the KNX component in Home Assistant introduce new functionalities, restructure existing code, and enhance the configuration management. Key changes include the introduction of a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Outside diff range and nitpick comments (2)
tests/components/knx/README.md (1)
Line range hint
1-73: The testing documentation is clear and detailed. Consider the following grammatical improvements for better readability:
- Line 5: Add a comma after "telegrams" before "To test something".
- Line 28 and 56: Vary the sentence structure to avoid repetition.
- Line 73: Replace "form" with "from".
- To test something add a test function requesting the `hass` and `knx` fixture and + To test something, add a test function requesting the `hass` and `knx` fixture and - Asserts that a GroupValueRead telegram was sent to `group_address`. + A GroupValueRead telegram was sent to `group_address`, this is asserted. - Inject and process a GroupValueWrite telegram addressed to `group_address`. + A GroupValueWrite telegram addressed to `group_address` is processed and injected. - for every Read-request sent form StateUpdater, or to pass its timeout, + for every Read-request sent from StateUpdater, or to pass its timeout,tests/components/knx/test_config_store.py (1)
92-108: Clarify the comment about the TTS platform.The comment on line 96 could be clearer. Consider rephrasing to "The 'tts' platform is not supported for KNX entities and is unlikely to ever be."
| except ConfigStoreException as err: | ||
| connection.send_error( | ||
| msg["id"], websocket_api.const.ERR_HOME_ASSISTANT_ERROR, str(err) | ||
| ) | ||
| return |
There was a problem hiding this comment.
Tip
Codebase Verification
Exception handling in ws_create_entity is not covered by tests. Please add tests to ensure this path is properly tested.
homeassistant/components/knx/websocket.pylines 283-287
Analysis chain
Ensure exception handling in ws_create_entity is covered by tests.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that exception handling paths in `ws_create_entity` are covered by tests.
# Test: Search for tests covering lines 283-287 in `ws_create_entity`.
rg --type python --context 5 'ws_create_entity' homeassistant/components/knx/tests/
Length of output: 114
Script:
#!/bin/bash
# Description: Verify that exception handling paths in `ws_create_entity` are covered by tests.
# Test: Search for tests covering lines 283-287 in `ws_create_entity`.
rg --type py --context 5 'ws_create_entity' homeassistant/components/knx/tests/
Length of output: 155
Script:
#!/bin/bash
# Description: Locate test files for the KNX component and verify that exception handling paths in `ws_create_entity` are covered by tests.
# Step 1: Locate test files related to the KNX component.
test_files=$(fd --type f --glob '*test*.py' homeassistant/components/knx/)
# Step 2: Search for tests covering `ws_create_entity` in the located test files.
if [ -n "$test_files" ]; then
rg --context 5 'ws_create_entity' $test_files
else
echo "No test files found for the KNX component."
fi
Length of output: 188
Script:
#!/bin/bash
# Description: Locate test files for the KNX component across the entire repository and verify that exception handling paths in `ws_create_entity` are covered by tests.
# Step 1: Locate test files related to the KNX component across the entire repository.
test_files=$(fd --type f --glob '*test*.py' | grep 'knx')
# Step 2: Search for tests covering `ws_create_entity` in the located test files.
if [ -n "$test_files" ]; then
rg --context 5 'ws_create_entity' $test_files
else
echo "No test files found for the KNX component."
fi
Length of output: 3805
| @@ -315,6 +335,7 @@ | |||
| async def start(self) -> None: | |||
| """Start XKNX object. Connect to tunneling or Routing device.""" | |||
| await self.project.load_project() | |||
| await self.config_store.load_data() | |||
There was a problem hiding this comment.
Ensure data loading in KNXModule.start() is robust and error-handled.
Consider adding error handling around the data loading calls in KNXModule.start() to manage potential failures gracefully. This could improve the robustness of the module startup process.
|
Really looking forward seeing this PR in the release soon :) |
farmio
force-pushed
the
knx-entity-creation
branch
from
0453857 to
f08ca44
Compare
| hass: HomeAssistant, config_entry: ConfigEntry, device_entry: DeviceEntry | ||
| ) -> bool: | ||
| """Remove a config entry from a device.""" | ||
| knx_module: KNXModule = hass.data[DOMAIN] |
There was a problem hiding this comment.
If you feel extra fancy, maybe you can do https://developers.home-assistant.io/blog/2024/05/01/improved-hass-data-typing (but in a separate PR of course)
There was a problem hiding this comment.
I do fancy type safety for sure!
Maybe I can even use runtime_data, but for now I'm trying to avoid merge conflict hell 😬 (I have some follow-ups ready locally 🙃)
| }, | ||
| extra=vol.ALLOW_EXTRA, | ||
| ), | ||
| msg="One of `Device` or `Name` is required", |
There was a problem hiding this comment.
the fields in there are device_info and name right? Is there a reason we call them Device and Name here? The backticks will make them look like they are code, but apparently they are not
There was a problem hiding this comment.
This refers to the 2 selectors in the frontend. One is a custom device selector (like an area selector where you can select a knx device, or create a new), the other is the entity name. The name selector is prefixed with the device name (in UI, not in schema). If there is no device selected, a user has to enter some entity name, if there is a device, it can be None or empty (_attr_has_entity_name).
Here with the error shown:
There was a problem hiding this comment.
Hmm, it's nasty because they can be translated
There was a problem hiding this comment.
The custom panel doesn't have translation in place yet. I guess this can be worked on when translation was implemented... for other things we use voluptuous error messages which are english too.
| from ..validation import ga_validator, maybe_ga_validator | ||
|
|
||
|
|
||
| class GASelector: |
There was a problem hiding this comment.
It is a common (in the KNX world) abbreviation for "Group Address". That's used to assign KNX telegram payloads to specific entity functions - and thus the main thing we need a user to configure (1-2 for each function - set and state address). Thats vaguely comparable to an MQTT topic or Modbus register address.
|
|
||
| async_add_entities(KNXSwitch(xknx, entity_config) for entity_config in config) | ||
| yaml_config: list[ConfigType] | None | ||
| if yaml_config := hass.data[DATA_KNX_CONFIG].get(Platform.SWITCH): |
There was a problem hiding this comment.
Side note: do we have multiple places within hass.data where we store KNX data? Ideally we only use the hass.data[DOMAIN]
There was a problem hiding this comment.
Yeah, there is the yaml config in DATA_KNX_CONFIG and an instance of the base class KNXModule in DOMAIN. I recall there was a reason why we did it that way, but tbh I can't remember why anymore 😬 had something to do with config entry reloads 🤔
This should be addressed together with typing improvements of hass.data I guess.
class can be used to add custom serializer later
farmio
force-pushed
the
knx-entity-creation
branch
from
28cf36d to
d0fc065
Compare
|
FYI pushed a new commit to fix the mypy errors / type ignores. @joostlek messaged me on discord about #104079 (comment) and I figured it'd probably be easier to push the changes directly then leaving a long comment. The gist of it though is to use the |
|
That was the last thing remaining for this PR. Much thanks @cdce8p |
Proposed change
Create, update and delete KNX entities from Websocket commands.
This is the base for creating KNX entities from UI. The related frontend PR is found here: XKNX/knx-frontend#112 . Once this is merged the frontend can be released so a dependency upgrade can set this working.
Type of change
Additional information
Checklist
black --fast 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..coveragerc.To help with the load of incoming pull requests: