-
-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix include_subclass union_strategy typing #431
Fix include_subclass union_strategy typing #431
Conversation
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. |
It seems like the tagged union strategy only uses methods on 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 |
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. |
435c8e7
to
b299cc9
Compare
No worries! I have included both changes and I checked with mypy that no new errors emerged. |
Sweet! Can you add a small line to HISTORY? |
Using TypeVar instead of always BaseConverter as one of the arguments to union_strategy allows more strategies to be passed. For example, if one requires a Converter for their strategy, this changes allows that strategy to be based to include_subclasses.
b299cc9
to
074fa61
Compare
074fa61
to
756111d
Compare
I missed your last comment! Should be ready now. |
Thanks! |
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.