Simplify model definitions with attrs converters and declarative patterns by iMicknl · Pull Request #1991 · iMicknl/python-overkiz-api

iMicknl · 2026-04-17T13:03:14Z

Summary

Replace manual __init__ methods with _flexible_init decorator + attrs converters across all models (Device, DeviceIdentifier, Location, Setup, Gateway, etc.)
Add _to_list, _to_optional, _to_optional_enum converter helpers to eliminate per-model boilerplate
Add dict index to CommandDefinitions and States containers for O(1) lookups
Device ui_class and widget are now public fields resolved at init time (no more private _ui_class/_widget with property wrappers)
Gateway.id and Place.id changed from mutable copies to read-only @property aliases
Remove unused _LOGGER, stale pylint comment, redundant one-off converters

Net reduction of ~300 lines while making every model follow the same declarative pattern.

Breaking changes

ServerConfig.type renamed to ServerConfig.api_type
Device.ui_class and Device.widget now return None instead of raising ValueError when no value is available
Gateway.id and Place.id are now read-only properties (no longer settable)
States.__getitem__ and CommandDefinitions.__getitem__ now raise KeyError instead of returning None (use .get() for the old behavior)

Test plan

All existing model tests pass
Device ui_class/widget resolve correctly from both direct kwargs and definition fallback
States/CommandDefinitions O(1) lookups work correctly
Gateway.id and Place.id properties return correct values

Copilot

Pull request overview

This PR refactors pyoverkiz model classes to use declarative attrs patterns with shared converters (and a decorator to ignore unknown API fields), and improves container lookup performance by adding internal indexes.

Changes:

Replace many manual model __init__ implementations with attrs fields + shared converter helpers and a _flexible_init decorator.
Add dict-backed indexes to States and CommandDefinitions to enable O(1) lookups and update tests accordingly.
Rename ServerConfig.type to ServerConfig.api_type and update client/auth/tests to use the new attribute.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`pyoverkiz/models.py`	Large refactor to declarative `attrs` models, shared converters, flexible init behavior, and indexed containers.
`pyoverkiz/client.py`	Update usage from `server_config.type` to `server_config.api_type`.
`pyoverkiz/auth/factory.py`	Update auth strategy selection/initialization to use `server_config.api_type`.
`tests/test_models.py`	Add coverage for new `States`/`CommandDefinitions` mapping behaviors; update `Action` construction style.
`tests/test_client.py`	Update assertions and `Action` construction to match model API changes (`api_type`, kw-only).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-17T13:10:11Z

+@_flexible_init
+@define(kw_only=True)
 class Action:
    """An action consists of multiple commands related to a single device, identified by its device URL."""

    device_url: str
-    commands: list[Command]
-
-    def __init__(self, device_url: str, commands: list[dict[str, Any] | Command]):
-        """Initialize Action from API data and convert nested commands."""
-        self.device_url = device_url
-        self.commands = [
-            c if isinstance(c, Command) else Command(**c) for c in commands
-        ]
+    commands: list[Command] = field(converter=_to_list(Command))



Action is now keyword-only, so Action(device_url, commands) positional construction no longer works (it did previously). If external callers rely on positional args, consider supporting both styles (e.g., a small custom __init__ shim) or adding an explicit deprecation note/release note.

Copilot · 2026-04-17T13:10:12Z

-
-
-@define(init=False, kw_only=True)
+    action_group: ActionGroup = field(converter=_to_optional("ActionGroup"))


Execution.action_group is annotated as non-optional ActionGroup, but it uses _to_optional(...) which can produce None. Either make the field optional with a default (and handle None), or use a non-optional converter/validator so the runtime behavior matches the type hint.

Suggested change

action_group: ActionGroup = field(converter=_to_optional("ActionGroup"))

action_group: ActionGroup | None = field(

default=None, converter=_to_optional("ActionGroup")

)

Copilot · 2026-04-17T13:10:12Z

-    sub_type: GatewaySubType | None = None
-    id: str = field(repr=obfuscate_id)
    gateway_id: str = field(repr=obfuscate_id)
+    connectivity: Connectivity = field(converter=_to_optional(Connectivity))


Gateway.connectivity is annotated as non-optional Connectivity, but uses _to_optional(...) which returns None for None input. To keep typing/runtime consistent, either make the attribute Connectivity | None with a default, or use a converter that enforces non-None inputs.

Suggested change

connectivity: Connectivity = field(converter=_to_optional(Connectivity))

connectivity: Connectivity | None = field(

default=None, converter=_to_optional(Connectivity)

)

Copilot · 2026-04-17T13:10:12Z

 class ServerConfig:
    """Connection target details for an Overkiz-compatible server."""

-    server: Server | None
+    server: Server | None = field(default=None, converter=_to_optional_enum(Server))
    name: str
    endpoint: str
    manufacturer: str
-    type: APIType
+    api_type: APIType = field(converter=_to_optional_enum(APIType))
    configuration_url: str | None = None


Renaming ServerConfig.type to api_type is a breaking change for downstream callers. Consider providing a backwards-compatible alias (e.g., a read-only type property mapping to api_type) with a deprecation path, so existing integrations don’t break immediately.

Copilot · 2026-04-17T13:10:13Z

+# mypy: disable-error-code="misc"
+


# mypy: disable-error-code="misc" disables an entire category of mypy diagnostics for this whole module, which can hide real typing regressions. Consider narrowing this to specific lines/classes (e.g., targeted # type: ignore[...] or fixing the underlying annotations) so mypy still provides useful coverage for the rest of the file.

Suggested change

# mypy: disable-error-code="misc"

Copilot · 2026-04-17T13:10:13Z

+def _to_command_definitions(value: Any) -> Any:
+    """Converter: raw list -> CommandDefinitions, or passthrough."""
+    if isinstance(value, list):
+        return _resolve("CommandDefinitions")(value)
+    return value
+
+


_to_command_definitions is defined but never used. Please remove it to avoid dead code, or switch Definition.commands to use it so there’s a single, consistent converter implementation.

Suggested change

def _to_command_definitions(value: Any) -> Any:

"""Converter: raw list -> CommandDefinitions, or passthrough."""

if isinstance(value, list):

return _resolve("CommandDefinitions")(value)

return value

## Summary - ActionGroup `label` field now defaults to `""` via a `_to_str` converter instead of post-init fixup - Simplifies `__attrs_post_init__` to only handle id/oid resolution Depends on #1991 (model simplifications). ## Breaking changes - `ActionGroup.label` type changed from `str | None` to `str` (always a string, `None` input is converted to `""`) ## Test plan - [ ] ActionGroup tests pass with label=None, label="", and label="value"

…ation defaults - Replace manual __init__ methods with attrs converters/factories across all model classes, using _flexible_init decorator to handle unknown API keys. - Rename ServerConfig.type to api_type to avoid shadowing the builtin. Update all references in client.py, auth/factory.py, and tests. - Fix Location.__init__ bug where field() objects were used as parameter defaults instead of None (now fixed by removing manual __init__).

__getitem__ now raises KeyError on missing keys (standard Python mapping protocol). get() returns None for missing keys. __contains__ uses explicit name comparison instead of delegating to __getitem__.

Replace Any types with concrete types based on actual API payloads: metadata -> str, deleted_raw_devices_count -> int, protocol_type -> int.

Both containers used O(n) linear scans for __contains__, __getitem__, get, and select. These are called on hot paths during Home Assistant polling cycles across many devices.

…init Replace manual __init__ with declarative attrs fields and __attrs_post_init__. The ui_class and widget fields are now public, resolved at construction time from either the API kwargs or the definition fallback.

…rties

## Summary - ActionGroup `label` field now defaults to `""` via a `_to_str` converter instead of post-init fixup - Simplifies `__attrs_post_init__` to only handle id/oid resolution Depends on #1991 (model simplifications). ## Breaking changes - `ActionGroup.label` type changed from `str | None` to `str` (always a string, `None` input is converted to `""`) ## Test plan - [ ] ActionGroup tests pass with label=None, label="", and label="value"

The API returns `setupOID` which decamelizes to `setup_oid`, but the field was named `setupoid`. This caused `_flexible_init` to silently drop the value, making `Event.setup_oid` always None.

action_group uses _to_optional which can return None, but the type annotation was non-optional. Also add start_time, execution_type and execution_sub_type which are present in the API response but were silently dropped by _flexible_init.

The field uses _to_optional converter which returns None for None input, but the type annotation was non-optional Connectivity.

Definition.commands uses an inline lambda converter instead. This function was defined but never referenced anywhere.

Reorder class definitions so dependencies are defined before dependents, allowing converter helpers to reference actual classes instead of string names. Setup is moved to the bottom since it references nearly every other model. The only remaining string forward reference is Place.sub_places which is self-referential and unavoidable. Also adds section comments to group models by domain: state/command primitives, device/definition, execution/action groups, infrastructure, configuration, and UI profiles.

## Summary - ActionGroup `label` field now defaults to `""` via a `_to_str` converter instead of post-init fixup - Simplifies `__attrs_post_init__` to only handle id/oid resolution Depends on #1991 (model simplifications). ## Breaking changes - `ActionGroup.label` type changed from `str | None` to `str` (always a string, `None` input is converted to `""`) ## Test plan - [ ] ActionGroup tests pass with label=None, label="", and label="value"

iMicknl requested a review from tetienne as a code owner April 17, 2026 13:03

Copilot AI review requested due to automatic review settings April 17, 2026 13:03

iMicknl mentioned this pull request Apr 17, 2026

Simplify ActionGroup label default #1992

Merged

1 task

Copilot started reviewing on behalf of iMicknl April 17, 2026 13:04 View session

iMicknl added v2 enhancement New feature or request breaking labels Apr 17, 2026

Copilot AI reviewed Apr 17, 2026

View reviewed changes

iMicknl force-pushed the v2/model-simplifications branch from ba112f9 to 2dea7ff Compare April 17, 2026 14:30

iMicknl mentioned this pull request Apr 17, 2026

Migrate to cattrs for centralized model structuring #1994

Open

5 tasks

iMicknl added 16 commits April 17, 2026 17:10

Fix States/CommandDefinitions container semantics

5c6accc

__getitem__ now raises KeyError on missing keys (standard Python mapping protocol). get() returns None for missing keys. __contains__ uses explicit name comparison instead of delegating to __getitem__.

Type Event.metadata, deleted_raw_devices_count, and protocol_type

833da69

Replace Any types with concrete types based on actual API payloads: metadata -> str, deleted_raw_devices_count -> int, protocol_type -> int.

Add dict index to CommandDefinitions and States for O(1) lookups

a8ae00e

Both containers used O(n) linear scans for __contains__, __getitem__, get, and select. These are called on hot paths during Home Assistant polling cycles across many devices.

Simplify DeviceIdentifier to use __attrs_post_init__ for base_device_url

ed6ed15

Replace _to_sub_places with _to_list("Place") forward reference

dafab56

Replace _to_server_enum and _to_api_type with _to_optional_enum

06bb69d

Remove unused _LOGGER and stale pylint comment

683f1bb

Change Gateway.id and Place.id from mutable copies to read-only prope…

61bd1fd

…rties

Fix Event.setupoid field name to match decamelize output

c21e152

The API returns `setupOID` which decamelizes to `setup_oid`, but the field was named `setupoid`. This caused `_flexible_init` to silently drop the value, making `Event.setup_oid` always None.

Fix Gateway.connectivity type annotation to be optional

da230fd

The field uses _to_optional converter which returns None for None input, but the type annotation was non-optional Connectivity.

Remove unused _to_command_definitions function

2108da2

Definition.commands uses an inline lambda converter instead. This function was defined but never referenced anywhere.

iMicknl force-pushed the v2/model-simplifications branch from 8d7d472 to 632ca31 Compare April 17, 2026 15:11

iMicknl marked this pull request as draft April 19, 2026 12:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify model definitions with attrs converters and declarative patterns#1991

Simplify model definitions with attrs converters and declarative patterns#1991
iMicknl wants to merge 16 commits intov2/mainfrom
v2/model-simplifications

iMicknl commented Apr 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Copilot AI Apr 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants



		@define(init=False, kw_only=True)
		action_group: ActionGroup = field(converter=_to_optional("ActionGroup"))

-    action_group: ActionGroup = field(converter=_to_optional("ActionGroup"))
+    action_group: ActionGroup | None = field(
+        default=None, converter=_to_optional("ActionGroup")
+    )

-    connectivity: Connectivity = field(converter=_to_optional(Connectivity))
+    connectivity: Connectivity | None = field(
+        default=None, converter=_to_optional(Connectivity)
+    )

Conversation

iMicknl commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking changes

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iMicknl commented Apr 17, 2026 •

edited

Loading