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
feat: add Config.smart_union
option
#2092
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2092 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 25 21 -4
Lines 5109 4149 -960
Branches 1050 834 -216
==========================================
- Hits 5109 4149 -960
Continue to review full report at Codecov.
|
c5af320
to
ca4ad05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Humm, this looks good.
I just wonder if we should go the whole hog and implement SmartUnion
which does this then tries sensible next steps.
Having:
Union
StrictUnion
LaxUnion
SmartUnion
DecriminatedUnion
Isn't very zen of python
There should be one-- and preferably only one --obvious way to do it.
I know we can't avoid having multiple, but I think we should aim at one type of union which is useful in as many cases as possible -- the behaviour we'll adopt for Union
if v2.
For me that would be SmartUnion
which does this and then falls back to the current union behaviour or something smarter.
See the discussion on #1423, e.g. #1423 (comment)
While trying out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise looks good, but what did you think about my suggestion above of SmartUnion
?
I agree but I want to take more time to do it properly. And I also need to support schema like a regular union |
Okay, so do you think we should merge this or wait for those improvements? |
I reckon we should wait. I'll try to work on it soon but I need to put everything in perspective and add some code + tests for schema, mypy... |
👍 |
StrictUnion
typeStrictUnion
type
@samuelcolvin Some news regarding this issue!! 🎉 I took some time today to give it another go after the opened issue on this matter at noon. I reckon the best approach is to keep the We want to tackle simple cases like Here is a POC that works well with it: https://github.com/PrettyWood/pydantic/pull/66/files. But I don't want to add a library (especially when it's mine! 😆) randomly even though it would be an extra requirement and not a required one. |
StrictUnion
typeConfig.smart_union
option
0f85ff4
to
7b8918e
Compare
7b8918e
to
f4e1223
Compare
This is looking awesome, 🎉 I really want some of this functionality. I'm a bit worried about adding another dependency, particularly because:
I know this is really annoying, but I would prefer to avoid the extra dependency if at all possible? I would prefer to skip the second pass here or find another route. I'm welcome to input from others though...? I think overall, the second pass will be less important as pydantic becomes stricter in v2. |
@samuelcolvin to be honest adding the extra library was a quick win to test it. We can definitely just copy paste part of the logic directly in pydantic now or in v2 :) |
@PrettyWood makes sense. |
eeaca09
to
01515a5
Compare
01515a5
to
8bd2bc9
Compare
@djpugh I updated the doc and moved your section with default |
docs/usage/types.md
Outdated
!!! warning | ||
`typing.Union` also ignores order when [defined](https://docs.python.org/3/library/typing.html#typing.Union), | ||
so `Union[int, float] == Union[float, int]` which can lead to unexpected behaviour | ||
when combined with matching based on the `Union` type order. | ||
Please note that this can also be [affected by third party libraries](https://github.com/samuelcolvin/pydantic/issues/2835) | ||
and their internal type definitions and the import orders. | ||
|
||
As such, it is recommended that, when defining `Union` annotations, the most specific type is included first and | ||
followed by less specific types. In the above example, the `UUID` class should precede the `int` and `str` | ||
classes to preclude the unexpected representation as such: | ||
followed by less specific types. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PrettyWood - Suggest maybe flipping these two parts for now, as it comes across slightly confusing with the warning then the recommended approach (given the warning applies to the recommended approach), and then maybe refer to Smart Union as the recommended approach to resolve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with b3de1a3
See https://smokeshow.helpmanual.io/3f1a2w4h2n461d35541v/usage/types/#unions. WDYT?
I want to show some exposure on smart union because we got a lot of issues regarding this matter.
Furthermore it will probably be the default behaviour for v2
Co-authored-by: David J Pugh <6003255+djpugh@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM.
@PrettyWood I know this has been a lonnnnng time, so I can make the change here to get this merged, but I want to leave it for you to check you're happy with it even though it's a pretty trival change.
pydantic/fields.py
Outdated
@@ -935,6 +935,35 @@ def _validate_singleton( | |||
) -> 'ValidateReturn': | |||
if self.sub_fields: | |||
errors = [] | |||
|
|||
if is_union_origin(get_origin(self.type_)) and self.model_config.smart_union: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if is_union_origin(get_origin(self.type_)) and self.model_config.smart_union: | |
if self.model_config.smart_union and is_union_origin(get_origin(self.type_)): |
Should be quicker since it's just checking a bool, if that is false we skip the is_union_origin
check.
3251af7
to
060a762
Compare
@samuelcolvin updated! Please review 🙏 |
🎉 thank you so much. |
@PrettyWood I've been trying this in master and it crashes when class Dict1(TypedDict):
foo: str
class Dict2(TypedDict):
bar: str
class M(BaseModel):
d: Union[Dict2, Dict1]
class Config:
smart_union = True
M(d=dict(foo='baz')) # raises TypeError: isinstance() arg 2 must be a type, a tuple of types, or a union |
Change Summary
Following this comment, here is a proposal for a new option
smart_union
that changes current behaviour ofUnion
to avoid coercion if possible@samuelcolvin I'll update the doc if you're fine with it
Related issue number
A bunch of closed ones 😄
closes #453
closes #1423
closes #3196
closes #3365
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)