2.0.0a1#74
Conversation
…rectly (#45) The OptionsFlowHandler was using _user_input_to_modification_data with modification_original_data, which filtered out any values that matched the original. This meant that resetting an attribute back to its original value would produce an empty modification_data dict, effectively making the edit a no-op. Fix: in the OptionsFlow, pass None as modification_original_data so all non-None user-provided values are stored, regardless of whether they match the original. The original data comparison is only relevant during initial creation (ConfigFlow) to avoid storing unnecessary data.
Refactor Entry/Device handlers and simplify engine/config flow logic. Key changes: - entry_handler: use comprehensions for merged/revert/update kwargs, attach DT config entries to devices with add_config_entry_id, remove them on revert (guarding against deleting the device), and streamline external change handling. - engine: import EntryHandler, add typing hints, simplify get-active-entries lambdas and normalize branch checks for modification types. - utils: remove unused helpers (merge/conflict checks) and unused imports; keep only necessary helpers and types. - config_flow: remove now-unneeded merge checks and unused imports; minor schema formatting adjustments. - data/original_data_store: minor formatting and signature cleanups. These changes simplify logic for device/entity modifications, reduce duplicated checks, and tighten up code style and comprehensions.
- Remove unused `hass` param from `_async_seed_store_from_config_entry` (ARG001 ruff error) - Use `async_delay_save` in OriginalDataStore for debounced I/O instead of eager saves - Fix mypy type annotation errors in engine.py (handler vars, lambda inference via functools.partial) - Store affected ID snapshots in engine to fix async_on_entry_updated mutation issue - Guard merge flow against selecting target device as merge source - Replace test_device_listener.py / test_entity_listener.py with test_entry_handler.py" Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/9045abdc-a09b-413d-9c01-851ba27bf7f5 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
Refactor modification logic and simplify entry handling
Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/b37c146e-ca0d-494c-a0af-9654cbd896a3 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…fault option Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/efa8595e-2008-466e-aee7-470962ba5152 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…support feat: add entity_category support to entity modifications
…ifiers to device modifications Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/53fd009a-1a3c-487d-aa04-7ec2a3ec11ab Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…o satisfy hassfest translation key validation Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/0186196a-9bc7-480a-9613-6a94b0d9f0b2 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
… form render and config entry storage Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/3cfac61e-3865-40ed-891d-d43ba7377ac9 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…onfig/options flow Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/4a422895-2100-4379-901b-cba1812f99fd Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…erministically, validate format, add de translations Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/6d0abb27-67cf-4d22-ba48-8311a8d578a5 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/739a9b2c-b09b-418d-b37d-aceed2c05db2 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…attributes feat: Add configuration_url, model_id, entry_type, connections, and identifiers to device modifications
…eleted Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/7c23ac1f-e612-48c5-9c4e-b73d210c6b01 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…letion Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/fe835467-0b15-4d06-95bb-8df297109ebe Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…assignment Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/7ce33ac5-a5b3-4b6e-9fbf-31fcedf652f0 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
When a creation-device modification is disabled: - Remove the deleted device from MERGE entries' original_data so the merge no longer claims entities that were on the deleted device - Handle missing original data in DeviceHandler.async_revert gracefully - Skip creating device handlers for non-existent devices in async_on_entry_loaded Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/78c39a4f-2da8-4602-941b-a7e6dc26356c Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…references Fix dangling device_id references when a creation-modification is deleted
…e merge devices Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/f2f108ac-e8e4-47d0-ba03-f536561f9bfa Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
… removal Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/69cde88c-685a-4c3a-b822-61736e637020 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
…iguration-url Add URL validation, graceful missing-device handling with real-time listeners, and editable merge sources in options flow
There was a problem hiding this comment.
Pull request overview
This PR appears to be the first 2.x alpha cut, refactoring Device Tools to a new “engine + per-target handlers” architecture while adding broader device/entity modification capabilities and a comprehensive v1→v2 migration test suite.
Changes:
- Replace the prior “Modification + Listener” class hierarchy with
ModificationEngineorchestratingEntityHandler/DeviceHandler, backed by a persistentOriginalDataStore. - Expand config/option flows to support additional device fields (connections, identifiers, entry type, configuration URL) and entity category selection.
- Add extensive tests for migration, handler behavior, and engine unload cascade logic; remove legacy listener tests.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_migration.py | New high-coverage tests for v1→v2 config-entry migration behavior. |
| tests/test_entry_handler.py | New tests for EntityHandler / DeviceHandler listening, apply, and revert behavior. |
| tests/test_engine.py | New tests for ModificationEngine dependency detection and unload cascade handling. |
| tests/test_entity_listener.py | Removed tests for old listener architecture. |
| tests/test_device_listener.py | Removed tests for old listener architecture. |
| custom_components/device_tools/utils.py | Removes merge-conflict helper functions; keeps only remaining utilities. |
| custom_components/device_tools/translations/en.json | Updates UI strings for new fields and validation errors. |
| custom_components/device_tools/translations/de.json | Updates UI strings for new fields and validation errors (German). |
| custom_components/device_tools/original_data_store.py | Adds persistent store for original (pre-modification) device/entity registry data. |
| custom_components/device_tools/modification.py | Removes old Modification base class. |
| custom_components/device_tools/merge_modification.py | Removes old MergeModification implementation. |
| custom_components/device_tools/listener.py | Removes old generic listener base. |
| custom_components/device_tools/entry_modification.py | Removes old entry-modification base class. |
| custom_components/device_tools/entry_handler.py | Adds new per-entity/per-device handler implementations. |
| custom_components/device_tools/entity_modification.py | Removes old EntityModification implementation. |
| custom_components/device_tools/entity_listener.py | Removes old entity listener implementation. |
| custom_components/device_tools/engine.py | Adds new central ModificationEngine orchestrator and deletion handling. |
| custom_components/device_tools/device_modification.py | Removes old DeviceModification implementation. |
| custom_components/device_tools/device_listener.py | Removes old device listener implementation. |
| custom_components/device_tools/data.py | Switches runtime data to store engine + store instead of listeners/modifications map. |
| custom_components/device_tools/const.py | Adds new constants and extends modifiable attribute sets (device + entity). |
| custom_components/device_tools/config_flow.py | Extends config/options flows with new fields, nested sections, and validation. |
| custom_components/device_tools/init.py | Updates setup/entry lifecycle to use engine/store; implements v1→v2 migration helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| entity_registry = er.async_get(hass) | ||
| result: list[str] = [] | ||
|
|
||
| for entity_uid in entity_modification.get("entities", []): | ||
| entity = entity_registry.async_get(entity_uid) | ||
| if entity is None: | ||
| _LOGGER.warning( | ||
| "Entity %s not found during migration, skipping", | ||
| entity_uid, | ||
| ) | ||
| continue | ||
| if entity.device_id == device_id: |
There was a problem hiding this comment.
_resolve_assigned_entities() treats the 1.x stored values as entity-registry entry IDs (“UIDs”), but it looks them up via entity_registry.async_get(entity_uid). In Home Assistant, async_get() expects an entity_id (e.g. "sensor.temp"), while registry IDs require async_get_entry(). As a result, assigned entities may fail to migrate and end up missing in the new DEVICE modification. Consider using async_get_entry(entity_uid) (and optionally falling back to async_get for backwards compatibility if some installs stored entity_id strings).
There was a problem hiding this comment.
Are you sure that 1.x uses UIDs in that list? You have to look at the original code.
There was a problem hiding this comment.
Confirmed — 1.x stores entity_ids (e.g. sensor.temp) in the entities list, not registry entry UUIDs. The EntitySelector in the 1.x config flow returns entity_id strings, the EntityListener keyed its callbacks by entity_id, and the old migration code used the same entity_registry.async_get() call. So the current lookup with async_get(entity_uid) is correct — "entity_uid" in the variable name is just a bit misleading for those items.
| # MERGE modification: remove any source devices that no longer exist | ||
| if mod_type == ModificationType.MERGE: | ||
| original_data: dict[str, Any] = config_entry.data.get( | ||
| CONF_MODIFICATION_ORIGINAL_DATA, {} | ||
| ) | ||
| missing_sources = [ | ||
| device_id | ||
| for device_id in original_data | ||
| if device_registry.async_get(device_id) is None | ||
| ] | ||
| if missing_sources: | ||
| new_original_data = { | ||
| k: v for k, v in original_data.items() if k not in missing_sources | ||
| } | ||
| for missing_id in missing_sources: | ||
| warn_key = f"{config_entry.entry_id}_{missing_id}" | ||
| if warn_key not in self._warned_missing: | ||
| self._warned_missing.add(warn_key) | ||
| _LOGGER.warning( | ||
| "Device %s referenced by merge modification '%s' no longer " | ||
| "exists. Removing from merge sources.", | ||
| missing_id, | ||
| title, | ||
| ) | ||
| self._hass.config_entries.async_update_entry( | ||
| config_entry, | ||
| data={ | ||
| **config_entry.data, | ||
| CONF_MODIFICATION_ORIGINAL_DATA: new_original_data, | ||
| }, | ||
| ) | ||
| # config_entry is mutated in-place by HA; re-read data from it | ||
| config_entry = self._hass.config_entries.async_get_entry( | ||
| config_entry.entry_id | ||
| ) or config_entry |
There was a problem hiding this comment.
When handling missing source devices in a MERGE modification, the code updates CONF_MODIFICATION_ORIGINAL_DATA but does not also reconcile options[CONF_MODIFICATION_DATA][CONF_MERGE_DEVICE_IDS] (which is written during v1→v2 migration). This can leave the entry’s options and data out of sync (UI may still suggest/select removed device IDs). Consider updating CONF_MERGE_DEVICE_IDS in options at the same time (when present) to match the filtered sources.
There was a problem hiding this comment.
After migration we don't need CONF_MERGE_DEVICE_IDS anymore, do we? I think we can completely remove that at the end of the migration process for merge modifications.
There was a problem hiding this comment.
You're right — CONF_MERGE_DEVICE_IDS is not needed after migration because the options flow already falls back to list(modification_original_data.keys()) when it's absent. Fixed in d344c19: migration no longer writes it, and the engine now also strips any stale CONF_MERGE_DEVICE_IDS from options on entries that were migrated before this fix (when it removes missing MERGE sources).
| assert result == [] | ||
|
|
||
|
|
||
| class TestAsyncOnEntryUnloadedStripsStalDeviceId: |
There was a problem hiding this comment.
Typo in test class name: "TestAsyncOnEntryUnloadedStripsStalDeviceId" → "...StripsStaleDeviceId". Fixing this will make test output and grep/search more consistent.
| class TestAsyncOnEntryUnloadedStripsStalDeviceId: | |
| class TestAsyncOnEntryUnloadedStripsStaleDeviceId: |
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/EuleMitKeule/device-tools/sessions/d2c8e042-e762-42ce-b407-de6ab129c4c4 Co-authored-by: EuleMitKeule <47421406+EuleMitKeule@users.noreply.github.com>
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self._store.get_device(device_id) is None: | ||
| device = device_registry.async_get(device_id) | ||
| if device is not None: | ||
| original = { | ||
| k: v | ||
| for k, v in device.dict_repr.items() | ||
| if k in MODIFIABLE_ATTRIBUTES[ModificationType.DEVICE] | ||
| } | ||
| await self._store.async_set_device(device_id, original) |
There was a problem hiding this comment.
OriginalDataStore persists to JSON via Store.async_delay_save, but this code stores device.dict_repr values directly for all MODIFIABLE_ATTRIBUTES[DEVICE]. In Home Assistant, fields like connections/identifiers are typically (frozen)sets of tuples and entry_type is a DeviceEntryType enum, which are not JSON-serializable and will cause delayed save to fail. Normalize these fields (e.g., convert connections/identifiers to list-of-[type,value] pairs and entry_type to its .value string) before calling store.async_set_device, or exclude them from persisted original data and reconstruct when reverting.
| changes: dict[str, Any] = event.data.get("changes", {}) | ||
| new_data = device.dict_repr | ||
| external_changes = {key: new_data[key] for key in changes if key in new_data} | ||
|
|
||
| if external_changes: | ||
| await self._store.async_update_device(self._entry_id, external_changes) |
There was a problem hiding this comment.
external_changes is derived from device.dict_repr and written into OriginalDataStore. If the device registry update includes fields like connections/identifiers/entry_type, those values are likely non-JSON-serializable (sets/frozensets of tuples, DeviceEntryType enum) and will break Store persistence. Normalize these values before calling store.async_update_device (consistent with the normalization used for config-entry data in config_flow.py).
| @@ -365,6 +399,8 @@ async def _async_migrate_merge_modification( | |||
| ) | |||
| continue | |||
|
|
|||
| valid_merge_device_ids.append(merge_device_entry_id) | |||
|
|
|||
There was a problem hiding this comment.
The local variable valid_merge_device_ids is populated but never used. Consider removing it (or using it if you intended to persist the filtered merge device IDs somewhere) to avoid dead code and keep migration logic clear.
| # --------------------------------------------------------------------------- | ||
| # Tests — merge only | ||
| # --------------------------------------------------------------------------- | ||
| """Migration of entries with only merge_modification.""" | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_creates_merge_entry_with_device_ids(self, mock_hass): | ||
| """A single MERGE entry is created without CONF_MERGE_DEVICE_IDS in modification_data.""" | ||
| entry = _make_v1_config_entry(V1_MERGE_ONLY) |
There was a problem hiding this comment.
The "merge only" test cases appear indented under TestMigrateAttributeAndEntity (starting at the standalone triple-quoted string and subsequent test_* methods). This makes them methods of the wrong class and leaves a stray string literal in the class body, which is confusing and can hinder future maintenance. Consider moving these merge-only tests into their own TestMigrateMergeOnly class (or fixing indentation) so test grouping matches intent.
No description provided.