python-attrs / cattrs

Composable custom class converters for attrs, dataclasses and friends.
https://catt.rs
MIT License
791 stars 110 forks source link

Fix include_subclass union_strategy typing #431

Closed VincentVanlaer closed 7 months ago

VincentVanlaer commented 11 months ago

Using Converter instead of BaseConverter as one of the arguments to union_strategy allows more strategies to be passed. Specifically, the configure_tagged_union strategy currently requires a Converter according to its typing.

Tinche commented 11 months ago

Hm how about we change the tagged union strategy to take a BaseConverter instead? I don't remember why I made it take a Converter, maybe it's an easy fix.

VincentVanlaer commented 10 months ago

It seems like the tagged union strategy only uses methods on BaseConverter, so changing that seems fine. In the meantime I realized it might make more sense to type include_subclasses as

C = TypeVar("C", bound=BaseConverter)

def include_subclasses(
    cl: Type,
    converter: C,
    subclasses: Optional[Tuple[Type, ...]] = None,
    union_strategy: Optional[Callable[[Any, C], Any]] = None,
    overrides: Optional[Dict[str, AttributeOverride]] = None,
) -> None:

since the argument passed to the union strategy is the same as is passed to include_subclasses. But that might be overkill for this situation. Let me know whether you prefer just the change to the tagged union strategy or also for include_subclasses.

Tinche commented 9 months ago

I'd be ok with changing both!

Sorry for the delay, I was focused on getting 23.2 out the door which is now done.

VincentVanlaer commented 9 months ago

No worries! I have included both changes and I checked with mypy that no new errors emerged.

Tinche commented 9 months ago

Sweet! Can you add a small line to HISTORY?

VincentVanlaer commented 7 months ago

I missed your last comment! Should be ready now.

Tinche commented 7 months ago

Thanks!