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: make pydantic dataclass
decorator support built-in dataclass
#1817
feat: make pydantic dataclass
decorator support built-in dataclass
#1817
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1817 +/- ##
===========================================
+ Coverage 99.90% 100.00% +0.09%
===========================================
Files 21 21
Lines 4051 4094 +43
Branches 807 821 +14
===========================================
+ Hits 4047 4094 +47
+ Misses 3 0 -3
+ Partials 1 0 -1
Continue to review full report at Codecov.
|
dataclass
decorator now support built-in dataclass
dataclass
decorator support built-in dataclass
6203b3d
to
2d076e1
Compare
This is great! |
This is amazing! Need a hand with the last details before this is merged to master? |
@hawkaa I'll update the doc once the behaviour is fully validated. If you want to help, you could review the code and ensure the behaviour is the right one even for nested dataclasses. I'm not sure I thought about everything there may probably be some edge cases to handle. |
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.
This looks awesome, I really want it.
Mostly small questions or tweaks.
return _process_class(cls, init, repr, eq, order, unsafe_hash, frozen, config) | ||
|
||
if _cls is None: | ||
return wrap | ||
|
||
return wrap(_cls) | ||
|
||
|
||
def make_dataclass_validator(_cls: Type[Any], **kwargs: Any) -> 'CallableGenerator': |
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.
docstring please to explain to what this does.
pydantic/dataclasses.py
Outdated
@@ -1,23 +1,32 @@ | |||
from typing import TYPE_CHECKING, Any, Callable, Dict, Generator, Optional, Type, TypeVar, Union | |||
import dataclasses |
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 have a suspicion that importing dataclasses
globally had a big effect on overall import time.
I think easiest to move it back to imports in functions, we could benchmark to see the performance of making this global.
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.
Yes I know I remember when you changed some imports to be local in order to improve overall import time.
I forgot to import it back to all functions.
pydantic/dataclasses.py
Outdated
@@ -128,18 +203,23 @@ def dataclass( | |||
unsafe_hash: bool = False, | |||
frozen: bool = False, | |||
config: Type[Any] = None, | |||
) -> Union[Callable[[Type[Any]], 'DataclassType'], 'DataclassType']: | |||
) -> Union[Callable[[Type[Any]], Type['DataclassType']], Type['DataclassType']]: |
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.
Wait, surely this signature is now wrong, you have double Type
and the type of a type is always just type
.
Maybe DataclassType
is defined wrongly?
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.
The name was missleading as DataclassType
was refering to an instance of a pydantic dataclass. I renamed it to Dataclass
.
pydantic/dataclasses.py
Outdated
return not hasattr(_cls, '__processed__') and dataclasses.is_dataclass(_cls) | ||
|
||
|
||
def _dataclass_with_validation( |
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.
not sure it's worth this being a separate function?
@@ -565,6 +567,9 @@ def find_validators( # noqa: C901 (ignore complexity) | |||
if is_literal_type(type_): | |||
yield make_literal_validator(type_) | |||
return | |||
if is_builtin_dataclass(type_): | |||
yield from make_dataclass_validator(type_) |
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.
👍 looks good.
The name `DataclassType` was missleading as it's not a `Type` per say.
pydantic import time was improved in pydantic#1132 by keeping `dataclass` import local. So let's keep it that way!
748f3d7
to
11397ec
Compare
@samuelcolvin Thanks for the review! I took all your remarks into account. I still need to write some documentation about this but first I really want to make sure the whole behaviour is right. If you think it is please let me know! |
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.
This looks great, just a small question, and documentation.
|
||
return cls | ||
|
||
|
||
@overload |
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.
No biggy, but make these should be in TYPE_CHECKING
?
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 tried but doesn't seem to work
And then I saw that it was not in TYPE_CHECKING
for root_validator
81cc288
to
5b0b889
Compare
5b0b889
to
e9d14eb
Compare
@samuelcolvin Documentation has been added 👍 Do not hesitate to tweak it if need be |
this is great, thanks. |
Change Summary
It was not possible to use
pydantic.dataclasses.dataclass
decorator directly on a built-indataclass
.Now it should work.
It allows to easily support nested dataclasses and add validation for them. This is why it's also part of this PR
ℹ️ I'll add some doc if the behaviour is validated and no extra use cases are added
Related issue number
closes #744
closes #1743
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change(see changes/README.md for details)