Skip to content
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

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
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
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.

2 participants