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

Use union_schema with Type[Union[...]] #6952

Merged
merged 2 commits into from Aug 1, 2023

Conversation

JeanArhancet
Copy link
Contributor

@JeanArhancet JeanArhancet commented Jul 30, 2023

Change Summary

Use union_schema and not is_subclass_schema if it's Type[Union[...]]

Related issue number

Resolves #6815

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @adriangb

@JeanArhancet JeanArhancet force-pushed the fix/type-union branch 2 times, most recently from 5fe10f4 to 4d94523 Compare July 30, 2023 13:27
@JeanArhancet JeanArhancet marked this pull request as ready for review July 30, 2023 13:41
@JeanArhancet JeanArhancet changed the title Fix Type union Use is_instance_schema with Type[Union[...]] Jul 30, 2023
@JeanArhancet
Copy link
Contributor Author

please review

@adriangb
Copy link
Member

adriangb commented Aug 1, 2023

I'm not sure that isinstance vs issubclass is the issue (although if it is flipped that would be a bug so this PR may make sense anyway).

The bigger issue IMO is that isinstance and issubclass are not meant to work with parametrized generics and unions. If you do issubclass(int, int|str) you get an error and isinstance(int, int|str) is False.

I think the best we can do (and what I'm guessing V1 did) is check if the thing inside of type[...] is a union and if so convert it to a union of types.

@JeanArhancet JeanArhancet changed the title Use is_instance_schema with Type[Union[...]] Use union_schema with Type[Union[...]] Aug 1, 2023
@JeanArhancet
Copy link
Contributor Author

@adriangb I changed the implementation WDYT?

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Looks good to me. While it is not a universal solution this is a relatively common use case and handling it is just a couple lines of code, so it seems worth it to me.

@adriangb adriangb enabled auto-merge (squash) August 1, 2023 14:54
@adriangb adriangb merged commit e86b529 into pydantic:main Aug 1, 2023
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Field of Type[Union[...]] fails in V2
2 participants