Skip to content

Conversation

@tetienne
Copy link
Collaborator

@tetienne tetienne commented Jan 9, 2026

Summary

  • Adds cattrs as a new dependency to handle structured dict→model conversion
  • Simplifies Partner, Connectivity, and Location models by eliminating duplicate field declarations in __init__ methods
  • Reduces ~56 lines of repetitive code while maintaining backward compatibility

Why

The current models.py has significant boilerplate where fields are declared twice - once in the class body and again in custom __init__ methods. Using cattrs with forbid_extra_keys=False elegantly handles the **_ pattern for ignoring unknown API fields, allowing attrs to generate the __init__ automatically.

This is a proof-of-concept that demonstrates the pattern before broader migration to remaining models.

@tetienne tetienne marked this pull request as ready for review January 9, 2026 14:24
@tetienne tetienne requested a review from iMicknl as a code owner January 9, 2026 14:24
@iMicknl iMicknl requested a review from Copilot January 11, 2026 16:38
@iMicknl
Copy link
Owner

iMicknl commented Jan 11, 2026

This is a proof-of-concept that demonstrates the pattern before broader migration to remaining models.

Thanks @tetienne! I was also thinking about migrating to Pydantic 2, as we want to convert JSON to classes (and vice versa). See #1862. What are your thoughts here?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors three model classes (Location, Partner, Connectivity) to use cattrs for automatic structuring from dictionaries, eliminating boilerplate __init__ methods. A new converters.py module is introduced with a pre-configured cattrs converter that ignores extra API keys.

Changes:

  • Added cattrs dependency (≥24.1.0) to handle dict-to-model conversion
  • Created converters.py module with cattrs converter and enum structure hook infrastructure
  • Refactored Location, Partner, and Connectivity classes to use attrs-generated __init__ methods instead of custom implementations

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.

File Description
uv.lock Added cattrs 25.3.0 dependency and updated tox to 4.34.0
pyproject.toml Added cattrs>=24.1.0 to project dependencies
pyoverkiz/converters.py New module providing a cattrs converter configured to ignore extra keys, with enum structure hooks for future model migrations
pyoverkiz/models.py Refactored Location, Partner, and Connectivity to use attrs-generated init; updated Setup and Gateway to use converter.structure() for creating nested model instances; fixed type annotations for optional Location fields

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

Comment on lines +25 to +39
def configure_converter() -> None:
"""Register structure hooks for all pyoverkiz enums.
This must be called after importing the enums module to ensure all enum
classes are available for registration.
"""
from pyoverkiz import enums

# Exclude base enum classes that are re-exported from the enums module
base_classes = (Enum, IntEnum, StrEnum)

for name in dir(enums):
obj = getattr(enums, name)
if isinstance(obj, type) and issubclass(obj, Enum) and obj not in base_classes:
converter.register_structure_hook(obj, _structure_enum)
Copy link

Copilot AI Jan 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enum structure hooks registered in configure_converter are not currently used by the refactored models (Location, Partner, Connectivity don't have enum fields). While this is fine as preparation for future refactoring, the current implementation may not correctly handle optional enum fields (e.g., UpdateBoxStatus | None). Consider adding tests for enum structuring before migrating models with enum fields, or add a hook for Optional[Enum] types using cattrs.register_structure_hook for Union types.

Copilot uses AI. Check for mistakes.
@tetienne
Copy link
Collaborator Author

This is a proof-of-concept that demonstrates the pattern before broader migration to remaining models.

Thanks @tetienne! I was also thinking about migrating to Pydantic 2, as we want to convert JSON to classes (and vice versa). See #1862. What are your thoughts here?

I agree that Pydantic 2 looks appealing. But:

  • We already use attrs. cattrs is ~40 lines of converter code vs rewriting 27 models.
  • cattrs adds ~0.4 MB vs Pydantic 2's ~6 MB (Rust binaries).
  • Pydantic excels at validation-heavy workloads. pyoverkiz is an API client that trusts its source, we need efficient parsing, not strict validation.
  • Pydantic would force us to be more rigid with our models and/or init.
  • For serialization, Command(dict) already works well. commands serialize automatically because they are dicts. Pydantic would require explicit .model_dump() calls.

@iMicknl
Copy link
Owner

iMicknl commented Jan 24, 2026

Agree, @tetienne! Lets see how we can improve the (especially v2) code to make it easier and more robust, but keeping the package lightweight.

@tetienne
Copy link
Collaborator Author

@iMicknl Do you want to merge this pr, and then let me continue the refactor?

@iMicknl
Copy link
Owner

iMicknl commented Jan 25, 2026

I will need to review the converters.py‎ code first, I don't fully understand yet what is happening.

This seems like quite a big change, perhaps we should see if we can add a few more tests to make sure we don't break existing behavior. My last (small) change broke a lot of installations 👀.

Would you be willing to do this change on the v2 branch otherwise?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants