Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix include_subclass union_strategy typing #431

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

VincentVanlaer
Copy link
Contributor

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
Copy link
Member

Tinche commented Sep 25, 2023

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
Copy link
Contributor Author

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 Tinche added this to the 24.1 milestone Nov 17, 2023
@Tinche
Copy link
Member

Tinche commented Nov 17, 2023

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 VincentVanlaer force-pushed the fix-subclass-strategy-typing branch 2 times, most recently from 435c8e7 to b299cc9 Compare November 21, 2023 22:20
@VincentVanlaer
Copy link
Contributor Author

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

@Tinche
Copy link
Member

Tinche commented Nov 22, 2023

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.
@VincentVanlaer VincentVanlaer force-pushed the fix-subclass-strategy-typing branch from b299cc9 to 074fa61 Compare January 22, 2024 22:13
@VincentVanlaer VincentVanlaer force-pushed the fix-subclass-strategy-typing branch from 074fa61 to 756111d Compare January 22, 2024 22:14
@VincentVanlaer
Copy link
Contributor Author

I missed your last comment! Should be ready now.

@Tinche
Copy link
Member

Tinche commented Jan 23, 2024

Thanks!

@Tinche Tinche merged commit d5e455a into python-attrs:main Jan 23, 2024
8 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants